Last Comment Bug 340796 - [FIX]Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build being different)
: [FIX]Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build be...
Status: RESOLVED FIXED
: crash, fixed1.8.0.5, fixed1.8.1, regression
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P1 critical with 2 votes (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks: 326645
  Show dependency treegraph
 
Reported: 2006-06-08 02:29 PDT by Jesse Ruderman
Modified: 2011-06-09 14:58 PDT (History)
12 users (show)
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack trace (mac debug) (3.07 KB, text/plain)
2006-06-08 02:30 PDT, Jesse Ruderman
no flags Details
this should do it (91.62 KB, patch)
2006-06-08 07:33 PDT, timeless
neil: superreview-
Details | Diff | Splinter Review
Proposed fix (2.75 KB, patch)
2006-06-12 21:10 PDT, Boris Zbarsky [:bz]
jonas: review+
neil: superreview+
jonas: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
1.8 branch patch (2.76 KB, patch)
2006-06-14 20:16 PDT, Boris Zbarsky [:bz]
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-06-08 02:29:56 PDT
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 2006-06-08 02:30:39 PDT
Created attachment 224841 [details]
stack trace (mac debug)
Comment 2 Jesse Ruderman 2006-06-08 02:31:00 PDT
> 3. Launch trunk build from the command line.

I meant "Launch trunk *debug* build from the command line".
Comment 3 Jesse Ruderman 2006-06-08 06:32:52 PDT
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 2006-06-08 07:33:45 PDT
Created attachment 224859 [details] [diff] [review]
this should do it
Comment 5 neil@parkwaycc.co.uk 2006-06-08 15:57:55 PDT
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 neil@parkwaycc.co.uk 2006-06-10 16:02:06 PDT
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 2006-06-12 01:52:10 PDT
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 2006-06-12 12:56:42 PDT
mRootContent is null when we crash. Someone is probably calling ResumeWalk after calling Destroy().

Ben? Bryner? Neil?
Comment 9 Josh Aas 2006-06-12 12:57:34 PDT
bz suggested that this bug is fallout for bug 326645, someone might want to look into that
Comment 10 timeless 2006-06-12 14:44:11 PDT
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 [:bz] 2006-06-12 17:33:26 PDT
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 [:bz] 2006-06-12 20:48:01 PDT
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 [:bz] 2006-06-12 21:06:15 PDT
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] 2006-06-12 21:07:22 PDT
Nominating for 1.8.0.5 because this appears to be a regression from a fix we want (bug 326645)
Comment 15 Boris Zbarsky [:bz] 2006-06-12 21:10:21 PDT
Created attachment 225381 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2006-06-13 01:47:26 PDT
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 2006-06-13 03:44:53 PDT
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 [:bz] 2006-06-13 07:35:05 PDT
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] 2006-06-13 14:37:41 PDT
Please land this on trunk/1.8 branches soon for verification
Comment 20 Boris Zbarsky [:bz] 2006-06-13 14:49:16 PDT
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 [:bz] 2006-06-14 20:16:59 PDT
Created attachment 225666 [details] [diff] [review]
1.8 branch patch
Comment 22 Boris Zbarsky [:bz] 2006-06-14 20:18:10 PDT
Fixed on trunk and 1.8 branch.
Comment 23 Boris Zbarsky [:bz] 2006-06-14 20:18:34 PDT
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] 2006-06-15 14:14:32 PDT
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 [:bz] 2006-06-19 17:46:54 PDT
Fixed on 1.8.0 branch.

Note You need to log in before you can comment on or make changes to this bug.