Closed
Bug 1258576
Opened 8 years ago
Closed 5 years ago
crash in nsContentIterator::NextNode
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: khuey, Assigned: masayuki)
References
Details
(Keywords: crash, regression, Whiteboard: [leave open] btpp-active)
Crash Data
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
smaug
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
This bug was filed from the Socorro interface and is report bp-6ebfdee9-a158-4e93-92a0-c1bda2160315. ============================================================= parent is null here. We have an NS_WARN_IF above but we don't actually return. What's up with that?
Flags: needinfo?(bugs)
Comment 1•8 years ago
|
||
masayuki added the warning, and changed the code around that time.
Flags: needinfo?(bugs)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(masayuki)
Assignee | ||
Comment 2•8 years ago
|
||
Different from bug 123855, the reports have a lot of comments. I'll try to look for some hints to reproduce this (perhaps, I can work on this next week).
Assignee | ||
Comment 3•8 years ago
|
||
https://crash-stats.mozilla.com/report/index/b814ce0b-0d22-4bd6-8c0d-6693b2160324 > In my gmail tab, in a hangouts conversation, every time that I multi-click in the chat window text > area, this causes the browser to crash. https://crash-stats.mozilla.com/report/index/af2f30d1-57c2-4c69-974c-da8d92160328 > I was writting a mail on Yahoo https://crash-stats.mozilla.com/report/index/f07e83c0-8761-4272-846f-e47dd2160326 > was in my hotmail inbox and suddenly inbox disappeared---firefox had apparently shut down according > to a Mozilla Crash Reporter https://crash-stats.mozilla.com/report/index/a3615133-d742-4f25-9f68-2ab112160326 > I had e-mail and Los Angeles news of some type open. I highlighted 2 rows of info in my email, > and hit delete. CRASH! 3rd time now! And some people said that pressing delete key caused this bug.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 4•8 years ago
|
||
Hmm, looks like that this is a bug of nsHTMLEditor. The crashed iterated must be created in nsDOMIterator. nsDOMIterator instance is created by: * nsHTMLEditRules::AlignInnerBlocks() * nsHTMLEditRules::BustUpInlinesAtBRs() * nsHTMLEditRules::AdjustSpecialBreaks() nsHTMLEditRules::AfterEditInner() calls only nsHTMLEditRules::AdjustSpecialBreaks(). Then, the content iterator is initialized with nsHTMLEditRules::mDocChangeRange. This must be removed range, if so, we can meet this crash.
Assignee: nobody → masayuki
Blocks: 1218032
Status: NEW → ASSIGNED
Component: DOM → Editor
Keywords: regression
Version: unspecified → Trunk
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=207a466bf6ea
Assignee | ||
Comment 6•8 years ago
|
||
nsContentIterator isn't designed as working fine with a tree whose some nodes are being removed. In such case, NextNode() and PrevNode() meets orphan node (i.e., a node whose parent is nullptr). Then, nsContentIterator should mark it as "done". However, it should keep crashing if it's debug build for detecting bugs explicitly. Review commit: https://reviewboard.mozilla.org/r/43509/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43509/
Assignee | ||
Comment 7•8 years ago
|
||
The patch is just a wallpaper for protecting users from the crash. This is the safest way to fix on 45esr. I'll attach additional patches which won't be uplift. (Unfortunately, I've not found the STR yet...)
Whiteboard: [leave open]
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8736656 [details] MozReview Request: Bug 1258576 part.1 nsContentIterator should give up to find next/previous node if it reached the root node unexpectedly Oops, r? isn't in the summary...
Attachment #8736656 -
Flags: review?(bugs)
Comment 9•8 years ago
|
||
Comment on attachment 8736656 [details] MozReview Request: Bug 1258576 part.1 nsContentIterator should give up to find next/previous node if it reached the root node unexpectedly https://reviewboard.mozilla.org/r/43509/#review40449 Based on the code it should matter much whether null is returned, but it makes those method more correct, IMO. ::: dom/base/nsContentIterator.cpp:778 (Diff revision 1) > } > > // post-order > nsINode* parent = node->GetParentNode(); > - NS_WARN_IF(!parent); > + if (NS_WARN_IF(!parent)) { > + MOZ_ASSERT(parent, "The node is the root node but not the last node"); Why do we assert here? Or is this for trying to find a test case for this? ::: dom/base/nsContentIterator.cpp:780 (Diff revision 1) > // post-order > nsINode* parent = node->GetParentNode(); > - NS_WARN_IF(!parent); > + if (NS_WARN_IF(!parent)) { > + MOZ_ASSERT(parent, "The node is the root node but not the last node"); > + mIsDone = true; > + return node; Why do we want to return node here? Shouldn't return null? ::: dom/base/nsContentIterator.cpp:849 (Diff revision 1) > if (mPre) { > nsINode* parent = node->GetParentNode(); > - NS_WARN_IF(!parent); > + if (NS_WARN_IF(!parent)) { > + MOZ_ASSERT(parent, "The node is the root node but not the first node"); > + mIsDone = true; > + return aNode; Same here. Shouldn't we return null here.
Attachment #8736656 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > ::: dom/base/nsContentIterator.cpp:778 > (Diff revision 1) > > } > > > > // post-order > > nsINode* parent = node->GetParentNode(); > > - NS_WARN_IF(!parent); > > + if (NS_WARN_IF(!parent)) { > > + MOZ_ASSERT(parent, "The node is the root node but not the last node"); > > Why do we assert here? > Or is this for trying to find a test case for this? Yes. > ::: dom/base/nsContentIterator.cpp:780 > (Diff revision 1) > > // post-order > > nsINode* parent = node->GetParentNode(); > > - NS_WARN_IF(!parent); > > + if (NS_WARN_IF(!parent)) { > > + MOZ_ASSERT(parent, "The node is the root node but not the last node"); > > + mIsDone = true; > > + return node; > > Why do we want to return node here? > Shouldn't return null? because nsContentIterator::Prev() and nsContentIterator::Next() doesn't return set the current node to nullptr even if it's done. See here: https://dxr.mozilla.org/mozilla-central/rev/538d248fa252a4100082fd9bc3fdc08d322cda22/dom/base/nsContentIterator.cpp#951,957-960,967,973-976 I.e., nsContentIterator::GetCurrentNode() returns the last node or the first node even after it's done. Do you still think PrevNode() and NextNode() should return nullptr in the cases?
Flags: needinfo?(bugs)
Comment 11•8 years ago
|
||
Well, GetCurrentNode() returns null, if mIsDone == true. So probably doesn't matter much. But ok fine, we can return non-null.
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11) > Well, GetCurrentNode() returns null, if mIsDone == true. Oh, you're right! > So probably doesn't matter much. > But ok fine, we can return non-null. Okay, then, I'll keep returning the original node for compatibility with some other methods.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8167849e665a
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=510f1fe03fcb
Assignee | ||
Comment 16•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96477e3f67b
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8736656 [details] MozReview Request: Bug 1258576 part.1 nsContentIterator should give up to find next/previous node if it reached the root node unexpectedly Approval Request Comment [Feature/regressing bug #]: Bug 1215798 (probably, but not confirmed because nobody can reproduce this crash) [User impact if declined]: Uses may meet this crash during editing text. So, it might cause dataloss of inputting text. [Describe test coverage new/current, TreeHerder]: Landed on m-c. [Risks and why]: Low. This is not completely fixing the bug. This patch just avoids the crash. This patch tries to keep normal behavior even if it meets the irregular case, but if user meets the case, he/she may see odd behavior. [String/UUID change made/needed]: Nothing.
Attachment #8736656 -
Flags: approval-mozilla-beta?
Attachment #8736656 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
[Tracking Requested - why for this release]: Over 1,000 crash reports per week. And the patch just avoids the crash. So, must be enough safe for ESR. (I don't think that this bug should be fixed on ESR completely with following patches, but at least for now, we should avoid the crash.)
status-firefox-esr45:
--- → affected
tracking-firefox-esr45:
--- → ?
Comment on attachment 8736656 [details] MozReview Request: Bug 1258576 part.1 nsContentIterator should give up to find next/previous node if it reached the root node unexpectedly Crash fix, Aurora47+
Attachment #8736656 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox47:
--- → affected
status-firefox48:
--- → fixed
Comment 20•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce029b692d55
Comment 21•8 years ago
|
||
45 and 46 are also affected.
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
Comment 22•8 years ago
|
||
Comment on attachment 8736656 [details] MozReview Request: Bug 1258576 part.1 nsContentIterator should give up to find next/previous node if it reached the root node unexpectedly Crash fix, may also avoid data loss, please uplift for beta 10.
Attachment #8736656 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Whiteboard: [leave open] → [leave open] btpp-active
Reporter | ||
Comment 25•8 years ago
|
||
Bug 1264932 has a testcase.
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #25) > Bug 1264932 has a testcase. Thank you for the infomation! I'd try to understand what the actual cause of this bug.
Updated•8 years ago
|
Comment 27•8 years ago
|
||
part 1 pushed https://hg.mozilla.org/releases/mozilla-esr45/rev/4ed86d63c09a to THUNDERBIRD_45_VERBRANCH
status-thunderbird_esr45:
--- → fixed
tracking-thunderbird_esr45:
--- → 46+
Updated•8 years ago
|
Target Milestone: --- → mozilla48
Comment 29•5 years ago
|
||
Closing because no crashes reported for 12 weeks.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•