Closed Bug 1396520 Opened 7 years ago Closed 7 years ago

Crash in nsRange::IsNodeSelected

Categories

(Core :: DOM: Core & HTML, defect)

57 Branch
Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-0040d15a-395b-4a3a-b534-8b0660170903.
=============================================================

There are 49 crashes in nightly 57 starting with buildid 20170902220453. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1395701.

[1] https://hg.mozilla.org/mozilla-central/rev?node=c1522ab270db8e96b8bae2a97715fde3cfd65574
Flags: needinfo?(bzbarsky)
Whiteboard: [clouseau]
Just about all the crashes are crashes trying to read from address 0x38 on this line:

        maxRangeCount = std::max(maxRangeCount, selection->RangeCount());

The crashes seem to largely be on the "amd64" build arch.  0x38 is 56 bytes.  RangeCount does mRanges.Length().

The memory layout of Selection on 64-bit is:

  nsISelectionPrivate vtable -- 8 bytes
  nsWrapperCache vtable -- 8 bytes
  nsWrapperCache data members -- 16 bytes, I think.
  nsSupportsWeakReference vtable -- 8 bytes
  nsSupportsWeakReference data members -- 8 bytes

That's 48 bytes.  The next member is mRanges, and Length() looks at its mHdr member, which is the first thing in the .  So if "selection" is null I'd expect mRanges.Length() to dereference 0x30, not 0x38...

Nevertheless, having an null "selection" there is my best bet as to what's going on.  Maybe I missed a member somewhere...
Oh, of course.  Before mRanges we have the one-word mRefCnt member, so 0x38 is in fact the offset of mRanges in Selection.  So that part all checks out.

Anyway, before my changes in bug 1395701 we did this:

     for (auto iter = ranges->ConstIter(); !iter.Done(); iter.Next()) {
       nsRange* range = iter.Get()->GetKey();
       if (range->IsInSelection() && !range->Collapsed()) {

Note that range->IsInSelection() check there.  I changed that to:

     for (nsRange* range : *ranges) {
       MOZ_ASSERT(range->IsInSelection(),
                  "Why is this range registeed with a node?");
       if (!range->Collapsed()) {

and it's possible, I guess, that we're ending up in a situation where that MOZ_ASSERT fails, and the old IsInSelect() check would have kept us out of the if body.  In that situation, "selection" would be null, of course (IsInSelection() just checks range->mSelection, which is what we init "selection" to).

OK, so why could that assert fail?  All calls to RegisterCommonAncestor() are guarded on IsInSelection() or mSelection.  mSelection can be nulled out in the following places:

1) Unlink.  But we explicitly unregister before that point.
2) nsRange::DoSetRange if !newCommonAncestor.  But we should have unregistered by then too.
3) nsRange::SetSelection. Bug if !mSelection we unregister.

So I don't quite see how we can have a registered range with !IsInSelection().  Ehsan, Mats, do you have any bright ideas offhand?

I can obviously reinstate the IsInSelection() check just to stop the crash, but the fact that that can ever test false here looks really fishy.
Flags: needinfo?(mats)
Flags: needinfo?(ehsan)
No, I can't think of anything off the top of my head.  This check comes directly from bug 619273 as far as I can tell which changed many things, so it's not clear to me where this specific check came from...  :/
Flags: needinfo?(ehsan)
I agree that assertion should hold, but I have no clue why it doesn't.
I suggest we keep both the check and the assert for now and hope we get
a testcase for the assertion we can analyze.
Flags: needinfo?(mats)
BTW, many of the URLs for the submitted crashes so far seems to be
login/authentication pages.
This should really always test true, but apparently sometimes doesn't...
that's quite strange.  Hopefully the diagnostic asserts will help pin down how
it can happen.
Attachment #8906110 - Flags: review?(ehsan)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Attachment #8906110 - Flags: review?(ehsan) → review?(mats)
Comment on attachment 8906110 [details] [diff] [review]
Add back in the IsInSelection() check in IsNodeSelected

r=mats (don't forget to update the commit message)
Attachment #8906110 - Flags: review?(mats) → review+
Yep, updated, thank you!
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df83df29fd50
Add back in the IsInSelection() check in IsNodeSelected.  r=mats
https://hg.mozilla.org/mozilla-central/rev/df83df29fd50
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.