Closed
Bug 1396520
Opened 7 years ago
Closed 7 years ago
Crash in nsRange::IsNodeSelected
Categories
(Core :: DOM: Core & HTML, defect)
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)
4.46 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•7 years ago
|
Whiteboard: [clouseau]
Assignee | ||
Comment 1•7 years ago
|
||
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...
Assignee | ||
Comment 2•7 years ago
|
||
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)
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
BTW, many of the URLs for the submitted crashes so far seems to be login/authentication pages.
Assignee | ||
Comment 6•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•7 years ago
|
Attachment #8906110 -
Flags: review?(ehsan) → review?(mats)
Comment 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df83df29fd50
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•