Last Comment Bug 637621 - Crash [@ nsRange::IsValidBoundary] after selected node is GCed
: Crash [@ nsRange::IsValidBoundary] after selected node is GCed
Status: RESOLVED FIXED
: [sg:critical?][has patch]
: crash, testcase
Product: Core
Classification: Components
Component: Selection
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla2.0
Assigned To: Olli Pettay [:smaug]
: selection
:
:
: 326633
  Show dependency treegraph
 
Reported: 2011-03-01 04:12 PST by Jesse Ruderman
Modified: 2011-05-23 13:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
[@ nsRange::IsValidBoundary]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  -
  ---
  .17+
  .17-fixed
  .19+
  .19-fixed


Attachments
testcase (requires extension for GC) (247 bytes, text/html)
2011-03-01 04:12 PST, Jesse Ruderman
no flags Details
stack trace (41.26 KB, text/plain)
2011-03-01 04:13 PST, Jesse Ruderman
no flags Details
patch (798 bytes, patch)
2011-03-01 06:21 PST, Olli Pettay [:smaug]
jst: review+
jst: approval2.0+
clegnitto: approval1.9.2.17+
clegnitto: approval1.9.1.19+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2011-03-01 04:12:43 PST
Created attachment 515877 [details]
testcase (requires extension for GC)

1. Install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Load the testcase.

Crash [@ nsRange::IsValidBoundary]

http://hg.mozilla.org/mozilla-central/file/410519307e63/content/base/src/nsRange.cpp#l598 is the crashing line
Comment 1 Jesse Ruderman 2011-03-01 04:13:05 PST
Created attachment 515878 [details]
stack trace
Comment 2 Jesse Ruderman 2011-03-01 04:17:22 PST
Seems to be debug-only with this testcase, but that's probably just because opt is lucky and memory isn't overwritten.
Comment 3 Olli Pettay [:smaug] 2011-03-01 06:15:22 PST
patch coming.
Comment 4 Olli Pettay [:smaug] 2011-03-01 06:21:25 PST
Created attachment 515901 [details] [review]
patch

Clear(presContext) in nsTypedSelection::Collapse deletes range objects,
and the method is called Collapse(lastRange->GetEndParent(), lastRange->EndOffset());

So, the safest patch I can think is to just keep aParentNode alive.
I know, the caller should do it, but in this case this is just simpler and safer.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-03-01 11:35:23 PST
Not blocking, but we'll take the patch once reviewed.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-03-01 11:36:19 PST
Comment on attachment 515901 [details] [review]
patch

r+a=jst
Comment 7 Olli Pettay [:smaug] 2011-03-02 07:01:59 PST
http://hg.mozilla.org/mozilla-central/rev/ab7fd603bb9d
Comment 8 Olli Pettay [:smaug] 2011-03-02 09:42:16 PST
Need to test if also branches need fixing.
Comment 9 Daniel Veditz 2011-03-02 10:38:59 PST
The code being patched looks equivalent on the branches, assuming we need this and blocking next releases.
Comment 10 Olli Pettay [:smaug] 2011-03-02 11:18:18 PST
Comment on attachment 515901 [details] [review]
patch

Seems to apply cleanly (some fuzz) to branches.
Comment 11 Christian Legnitto [:LegNeato] 2011-03-09 10:48:02 PST
Comment on attachment 515901 [details] [review]
patch

a=LegNeato for 1.9.2.16 and 1.9.1.18
Comment 12 Daniel Veditz 2011-03-28 10:44:01 PDT
Can we get this landed on branches?
Comment 13 Olli Pettay [:smaug] 2011-03-29 08:56:12 PDT
Sorry, I need to land this asap.
Comment 15 Olli Pettay [:smaug] 2011-03-29 11:22:07 PDT
Had to land a bustage fix for 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fbb2dfda1784

I hadn't noticed that nsIDOMNode* parameter was changed to nsINode*

Note You need to log in before you can comment on or make changes to this bug.