340796 - [FIX]Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build being different)

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Jesse Ruderman]

Steps to reproduce, at least on my machine:
1. Launch 2006-06-07 nightly.
2. Close it.
3. Launch trunk build from the command line.

Result: Crash [@ nsXULDocument::ResumeWalk] after "WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Users/admin/trunk/mozilla/xpcom/base/nsExceptionService.cpp, line 191".

Comment 1

[Jesse Ruderman]

Attachment: stack trace (mac debug)

Comment 2

[Jesse Ruderman]

> 3. Launch trunk build from the command line.

I meant "Launch trunk *debug* build from the command line".

Comment 3

[Jesse Ruderman]

This happens with today's nightly too, so it's not limited to debug builds or due to changes I have in my tree.

Comment 4

[timeless]

Attachment: this should do it

Comment 5

[[e-mail]]

Comment on attachment 224859 [details] [diff] [review]
this should do it

I want the title to be set even if no attribute could be found. And if you're not going to do it, I want something that will remind people to find out why this is happening. And finally, I want to see peer review.

Comment 6

[[e-mail]]

OK, so seamonkey -mail -compose -addressbook triggers this presumably because they all try to load the new account wizard into the same DOM window.

Comment 7

[timeless]

neil: err, aren't you peer for xul? if not, who is?

note that i didn't claim ownership of this bug. my product doesn't even build xul, i have no affiliation to this bug or any others like it, this patch was posted as a one off.

Comment 8

[Josh Aas]

mRootContent is null when we crash. Someone is probably calling ResumeWalk after calling Destroy().

Ben? Bryner? Neil?

Comment 9

[Josh Aas]

bz suggested that this bug is fallout for bug 326645, someone might want to look into that

Comment 10

[timeless]

that makes sense as an explanation but afaik is entirely unhelpful about dealing with it. unless jesse wants to get a stack trace for destroy and post it here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well.  Dealing with stuff should be straightforward -- any code that assumes mRootContent is non-null is on crack and needs spanking.  I'll post a patch to that effect later tonight, probably.

What I want to know is whether it's considered a bug that ResumeWalk is being called after Destroy.... I'm not sure it is, but I'm not too clear on what the heck ResumeWalk really is, so...

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, with the steps in comment 6 and a vanilla profile, I can reproduce.  That also fires the following asserts (some of which look bogus, and some of which indicate major XUL bugs, imo):

###!!! ASSERTION: no script global object: 'mScriptGlobalObject != nsnull', file ../../../../../mozilla/content/xul/document/src/nsXULDocument.cpp, line 3186

(due to trying to ExecuteScript off an OnStreamComplete in a document that's already had its script global nulled out...  I wonder why that channel didn't get canceled via the loadgroup...)

###!!! ASSERTION: element has no document: 'doc', file ../../../../../mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp, line 342

(because we're hooking up template builders after we've nuked our DOM)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and also:

###!!! ASSERTION: Event listener added to outer window!: 'win->IsInnerWindow()', file ../../../../mozilla/content/events/src/nsEventListenerManager.cpp, line 1229

because GetScriptGlobalObject will return an outer window after teardown...

We should probably get bugs filed on all of these.

Comment 14

[Daniel Veditz [:dveditz]]

Nominating for 1.8.0.5 because this appears to be a regression from a fix we want (bug 326645)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I think this is the safe way to go.  Behavior is pretty wild with this change for the testcase in comment 6 (e.g. the account wizard is a 0x0 window), but it's better than crashing...

Comment 16

[[e-mail]]

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

>+            mRootContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::title,
>+                                  title);
Anything wrong with an 80-character line? ;-)

Comment 17

[timeless]

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

>@@ -3499,16 +3503,20 @@ nsXULDocument::OverlayForwardReference::
>     if (id.IsEmpty()) {

I don't think this comment matches the code that follows:
>         // overlay had no id, use the root element
>+        if (!mDocument->mRootContent) {
>+            return eResolve_Error;

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sure it matches.  No id, so use the root element as the insertion point.  Then duplicate the other insertion point logic -- no point means bail, otherwise insert.

Comment 19

[Daniel Veditz [:dveditz]]

Please land this on trunk/1.8 branches soon for verification

Comment 20

[Boris Zbarsky [:bzbarsky]]

I will once trunk reopens.  If that doesn't happen by tomorrow, I'll need someone to land this for me if it's going to make 1.8.0.5...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk and 1.8 branch.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 225666 [details] [diff] [review]
1.8 branch patch

This fixes a crash regrssion from a security fix we want on branch by adding needed null-checks.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 225666 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch.