Martijn Wargers (dead) Reporter
	Description • 19 years ago

See upcoming testcase, which crashes Mozilla after a second or so.
It also crashes Mozilla1.7.12.
Unfortunately, it's not a minimised testcase (which I haven't been able to make yet).

Talkback ID: TB14952503H

Somehow, it doesn't crash in my debug build, it just stops after the 'failed to load URL' assertion.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 19 years ago

The root cause of the crash is:

(gdb) frame 3
#3  0xb733e769 in nsContentList::PopulateSelf (this=0x884f420, aNeededLength=4294967295)
    at ../../../../mozilla/content/base/src/nsContentList.cpp:869
869           PopulateWith(root, PR_TRUE, elementsToAppend);
(gdb) p mDocument
$11 = (class nsIDocument *) 0x8746660
(gdb) p mDocument->GetRootContent()
$12 = (class nsIContent *) 0x8709228
(gdb) p mDocument->GetRootContent()->GetOwnerDoc()
$13 = (class nsIDocument *) 0x8746660
(gdb) p mDocument->GetRootContent()->GetDocument()
$14 = (class nsIDocument *) 0x0

That should not happen -- $14 should be the same as $13 and $11.  I'll see whether I can add some asserts that would let me catch this when the mismatch first appears.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 19 years ago

So nsDocument::Destroy was putting us into this bogus state...  This patch fixes that; the rest is window-dressing.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 19 years ago

Comment on attachment 211470 [details] [diff] [review]
Proposed fix

>Index: content/base/src/nsDocument.h
...
> #ifdef DEBUG
>-  void VerifyRootContentState();
>+  // aAllowBogusChild is kinda needed by nsDocument::Destroy and ~nsDocument.
>+  void VerifyRootContentState(PRBool aCompareToChildArray = PR_FALSE);
> #endif

You call it both "aAllowBogusChild" and "aCompareToChildArray" here. And neither is IMHO very descriptive :)

How about "aIgnoreMismatchedRoot"?


>Index: content/base/src/nsDocument.cpp
>@@ -743,26 +743,39 @@ nsDocument::~nsDocument()
...
>+      // Note: we want neither DOM mutations nor notifications happening here
>+      // (the latter wouldn't matter anyway, since we've cleared our
>+      // observers).
>       PRUint32 count = mChildren.ChildCount();
>       for (indx = PRInt32(count) - 1; indx >= 0; --indx) {
>+        nsIContent* content = mChildren.ChildAt(indx);
>+        if (content == mRootContent) {
>+          // Null out mRootContent first; this is similar to what
>+          // RemoveChildAt() does.
>+          mRootContent = nsnull;
>+        }
>         mChildren.ChildAt(indx)->UnbindFromTree();
>         mChildren.RemoveChildAt(indx);
>       }
>     }
>   }
> 
>   mRootContent = nsnull;

This shouldn't be needed any more, no? Especially since we should be able to remove the GetCurrentDoc() check just ouside the context.

And it really looks like we should remove the mRootContent check too. We always want to unbind all root content even if there's no documentElement.

>+#ifdef DEBUG
>+  VerifyRootContentState(PR_TRUE);
>+#endif

Why does this need to be PR_TRUE?

r=me with that fixed or explained.

	Peter Van der Beken [:peterv]
	Comment 5 • 19 years ago

(In reply to comment #4)
> This shouldn't be needed any more, no? Especially since we should be able to
> remove the GetCurrentDoc() check just ouside the context.
> 
> And it really looks like we should remove the mRootContent check too. We always
> want to unbind all root content even if there's no documentElement.

We do unbind all content, the nsAttrAndChildArray destructor takes care of that. We can't rely on it for elements because it passes PR_FALSE for aDeep to UnbindFromTree. Maybe we should make nsAttrAndChildArray::Clear take an aDeep argument and call it with PR_TRUE here? Calling RemoveChildAt is more expensive because it memmove's every time.

> >+#ifdef DEBUG
> >+  VerifyRootContentState(PR_TRUE);
> >+#endif
> 
> Why does this need to be PR_TRUE?

I don't understand why we need aCompareToChildArray/aAllowBogusChild anyway, since we now always seem to null out mRootContent?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 19 years ago

> I don't understand why we need aCompareToChildArray/aAllowBogusChild anyway,

Because it's there to assert that either mRootContent is null or it has the right current document.... I guess I could just assert that manually both in the relevant spots and in VerifyRootContentState and remove the boolean arg.

Passing a boolean to nsAttrAndChildArray::Clear sounds reasonable to me.  Sicking, what do you think?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 19 years ago

I was actually just referring to removing the |mRootContent = nsnull|. As well as removing the |if|s around the loop. But passing a bool to nsAttrAndChildArray::Clear seems ok to me.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 19 years ago

So the problem with just doing Clear() is that we want to unset mRootContent _before_ we do this stuff.  Think mutation event listeners that trigger during this code.  :(

	Peter Van der Beken [:peterv]
	Comment 9 • 19 years ago

(In reply to comment #8)
> So the problem with just doing Clear() is that we want to unset mRootContent
> _before_ we do this stuff.  Think mutation event listeners that trigger during
> this code.  :(

And setting mRootContent to nsnull before calling Clear wouldn't work?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 19 years ago

I'm not sure it would; it would produce very different ordering from what normally happens (eg when PIs are being unbound mRootContent would already be null, etc).

If you think it should work, please take the bug?  I'm not going to be able to work on this in the foreseeable future (till summer?) in any case.  :(

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 11 • 19 years ago

This fixes the inconsistent state by removing mRootContent. I removed mBodyContent while I was at it.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 19 years ago

Comment on attachment 214276 [details] [diff] [review]
Alternative patch

>Index: content/base/src/nsDocument.cpp
> nsDocument::InsertChildAt(nsIContent* aKid, PRUint32 

>   if (aKid->IsContentOfType(nsIContent::eELEMENT)) {
>+    if (GetRootContent()) {

Why not combine with && into a single if?

>Index: content/base/src/nsDocument.h

>+  nsIContent* GetRootContent() const;

  virtual nsIContent* GetRootContent() const;

>Index: content/html/document/src/nsHTMLDocument.cpp
>@@ -2434,41 +2419,42 @@ nsHTMLDocument::GetElementById(const nsA

>+    nsIContent* root = GetRootContent();
>     if (!IdTableIsLive()) {

I don't like that.  This will make us GetRootContent even when the table is live (in which case we _know_ there's nothing for us here, and the root content is only needed in a debug block.  So I'd put this GetRootContent() call inside the !IdTableIsLive() block.  And in the debug block further down, just reget it -- what do we care about an extra debug-only call?  ;)

> nsHTMLDocument::GetBodyContent()
>+  // Loop backwards because any non-elements, such as doctypes and PIs
>+  // are likly to appear before the root element.

s/likly/likely/

>+  for (i = mChildren.ChildCount(); i > 0; --i) {
>+    nsIContent* html = mChildren.ChildAt(i - 1);
>+    if (html->Tag() == nsHTMLAtoms::html &&
>+        html->IsContentOfType(nsIContent::eHTML)) {

Why not just GetRootContent() and then see whether it's the right thing?  For that matter, why are we adding the check that this is actually <html>?  We didn't use to do that...

>+      // Look for body inside html
>+      for (i = html->GetChildCount(); i > 0; --i) {

This reversed the search, so if there are two bodies there the answer changes from what it is now.  I realize this is not likely, but still, I'd prefer we didn't change that.

>Index: content/html/document/src/nsImageDocument.cpp
>@@ -656,17 +656,17 @@ nsImageDocument::CheckOverflowing(PRBool
>+  nsIContent* content = GetBodyContent();
>   nsRefPtr<nsStyleContext> styleContext =
>     context->StyleSet()->ResolveStyleFor(content, nsnull);

Add a null-check while you're here?  There's no guarantee that GetBodyContent() returns non-null.

>Index: content/svg/content/src/nsSVGSVGElement.cpp
>+  PRBool IsRoot() {
>+    return IsInDoc() && !GetParent();

Add an assert that this is the same as being equal to GetRootContent()?

r=bzbarsky, but I'd like jst to ok the removal of mBodyContent...

	Johnny Stenback (:jst)
	Comment 13 • 19 years ago

Comment on attachment 214276 [details] [diff] [review]
Alternative patch

Fine with me. sr=jst

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 19 years ago

Sicking, this is all yours.  ;)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 19 years ago

(In reply to comment #12)
> >+  for (i = mChildren.ChildCount(); i > 0; --i) {
> >+    nsIContent* html = mChildren.ChildAt(i - 1);
> >+    if (html->Tag() == nsHTMLAtoms::html &&
> >+        html->IsContentOfType(nsIContent::eHTML)) {
> 
> Why not just GetRootContent() and then see whether it's the right thing?  For
> that matter, why are we adding the check that this is actually <html>?  We
> didn't use to do that...

I added the check for html on purpose because IMHO that's the behaviour we want. There's another change in behaviour in this patch. We now honor non-xhtml bodies in xhtml docs (as well as the other way around). I talked with jst and he agreed that's what we want.

> >+      // Look for body inside html
> >+      for (i = html->GetChildCount(); i > 0; --i) {
> 
> This reversed the search, so if there are two bodies there the answer changes
> from what it is now.  I realize this is not likely, but still, I'd prefer we
> didn't change that.

Yeah, I agree, i changed it to loop forward.

> >Index: content/html/document/src/nsImageDocument.cpp
> >@@ -656,17 +656,17 @@ nsImageDocument::CheckOverflowing(PRBool
> >+  nsIContent* content = GetBodyContent();
> >   nsRefPtr<nsStyleContext> styleContext =
> >     context->StyleSet()->ResolveStyleFor(content, nsnull);
> 
> Add a null-check while you're here?  There's no guarantee that 
> GetBodyContent() returns non-null.

Not really sure what the right behaviour is if there's no body. I simply returned NS_OK

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 16 • 19 years ago

Checked in, thanks for reviews

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 19 years ago

The decl of VerifyRootContentState in nsDocument.h should go away too, no?

	David Baron :dbaron:
	Comment 18 • 19 years ago

This caused regression bug 330302 (probably by changing the behavior of nsHTMLDocument::GetBody somehow).

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 19 • 19 years ago

So this patched caused bug 330302 as well as a slight performance regression and so the patch is currently backed out.

The performance issue might be slightly helped by bug 331530.

A bigger problem is bug 330302 though. I'll talk with bryner about why we even have a nsDocument::Destroy function, it was added for the bfcache.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 20 • 19 years ago

This contains some slight modifications to save a few cycles, though probably not enough to make a difference.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 18 years ago

So we should really fix this, one way or another.   See also bug 335896 (which is basically caused by the same problems)...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 18 years ago

bz: So I think we should go with your patch here. Please check in whenever you can so that maybe we can get it on the 1.8.0 branch.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 18 years ago

Let's do the minimal patch that will fix this, then, and file a followup on making the world better...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 24 • 18 years ago

Comment on attachment 224627 [details] [diff] [review]
Minimal patch, updated to tip

Please land this as soon as possible so that we get some bake time before 1.5.0.5

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 18 years ago

Fixed on trunk and 1.8 branch.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 26 • 18 years ago

Verified FIXED using the testcase https://bugzilla.mozilla.org/attachment.cgi?id=211374 on a SeaMonkey trunk 2006-06-08-09 build under Windows XP.

	Daniel Veditz [:dveditz]
	Comment 27 • 18 years ago

Comment on attachment 224627 [details] [diff] [review]
Minimal patch, updated to tip

approved for 1.8.0 branch, a=dveditz for drivers

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 18 years ago

This probably exposed bug 340796.  I'm holding off on landing this on the 1.8.0 branch until I get an approved fix for that...

	Adam Guthrie
	Comment 29 • 18 years ago

*** Bug 341315 has been marked as a duplicate of this bug. ***

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 18 years ago

*** Bug 341050 has been marked as a duplicate of this bug. ***

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 31 • 18 years ago

This looks like it also exposed bug 341730...

I'm going to try to come up with a completely safe null-check-only patch for the 1.8.0.x branch here.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 32 • 18 years ago

OK, the null-check thing does not help that much on the 1.8.0 branch.  It does fix this crash, but then we crash messing with what looks like a deleted nsAttributeChildList from JS(???).  I get that result even with the patch in this bug (which _does_ fix the crash on trunk).

Is it still worth getting the null-check in on 1.8.0.5?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 33 • 18 years ago

So it might be worth taking it on the 1.5.0.5 tree since we could maybe fix the later crash in a further release. I'm not sure if it's worth it though given that we're transitioning to FF2 in a not too distant future.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 34 • 18 years ago

Again, this does not fully fix things on branch; if fixes the crash that used to happen, but now another one happens...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 35 • 18 years ago

Comment on attachment 226222 [details] [diff] [review]
Null-check-only patch -- for 1.8.0.x branch only

We decided not to take this since the current crash is an unexploitable null-pointer deref, whereas the new crash is messing with a deleted object which could be worse.

	Daniel Veditz [:dveditz]
	Comment 36 • 18 years ago

If we can work out the regressions maybe this can make 1.8.0.6

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 37 • 18 years ago

Pushing this out to 1.8.0.6 in case we get a fix for this and the later crashes. Though I'm not sure how vital this is since it seems like this is "just" a nullpointer deref caused by fuzzing.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 38 • 18 years ago

OK, makes sense.  For what it's worth, I see the same crash on 1.8.x as on 1.8.0.x...  Bug 342085 filed on that; I'll try to come up with a patch there.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 39 • 18 years ago

Comment on attachment 226222 [details] [diff] [review]
Null-check-only patch -- for 1.8.0.x branch only

OK, with the "null-check-only patch" and the patch for bug 342085 (which should be really really safe) I no longer crash on this testcase on both 1.8 and 1.8.0 branches.  Given that, I think I'd like to rerequest 1.8.0.5 approval on the null-check.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 40 • 18 years ago

Comment on attachment 226222 [details] [diff] [review]
Null-check-only patch -- for 1.8.0.x branch only

great! r/sr=sicking
a=sicking for drivers

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 41 • 18 years ago

Fixed on the 1.8.0 branch.  Thanks for the speedy review!

	Tracy Walker [:tracy]
	Comment 42 • 18 years ago

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060620 Firefox/1.5.0.5