Closed Bug 1258576 Opened 4 years ago Closed 17 days ago

crash in nsContentIterator::NextNode

Categories

(Core :: Editor, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED WORKSFORME
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 - wontfix
thunderbird_esr45 46+ fixed

People

(Reporter: khuey, Assigned: masayuki)

References

Details

(Keywords: crash, regression, Whiteboard: [leave open] btpp-active)

Crash Data

Attachments

(1 file)

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)
masayuki added the warning, and changed the code around that time.
Flags: needinfo?(bugs)
Flags: needinfo?(masayuki)
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).
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)
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
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/
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]
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 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+
(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)
Well, GetCurrentNode() returns null, if mIsDone == true. So probably doesn't matter much.
But ok fine, we can return non-null.
Flags: needinfo?(bugs)
(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.
Blocks: 1215798
No longer blocks: 1218032
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?
[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.)
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+
45 and 46 are also affected.
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+
Whiteboard: [leave open] → [leave open] btpp-active
(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.
Duplicate of this bug: 1253855
Target Milestone: --- → mozilla48

Closing because no crashes reported for 12 weeks.

Status: ASSIGNED → RESOLVED
Closed: 17 days ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.