Closed Bug 340796 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: jruderman, Assigned: bzbarsky)

References

()

Details

(4 keywords)

Crash Data

Attachments

(3 files, 1 obsolete file)

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".
> 3. Launch trunk build from the command line.

I meant "Launch trunk *debug* build from the command line".
This happens with today's nightly too, so it's not limited to debug builds or due to changes I have in my tree.
Flags: blocking1.9a1?
Keywords: regression
Summary: Crash during restart (due to last build being different) → Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build being different)
Attached patch this should do it (obsolete) — Splinter Review
Attachment #224859 - Flags: superreview?(neil)
Attachment #224859 - Flags: review?(neil)
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.
Attachment #224859 - Flags: superreview?(neil)
Attachment #224859 - Flags: superreview-
Attachment #224859 - Flags: review?(neil)
OK, so seamonkey -mail -compose -addressbook triggers this presumably because they all try to load the new account wizard into the same DOM window.
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.
mRootContent is null when we crash. Someone is probably calling ResumeWalk after calling Destroy().

Ben? Bryner? Neil?
bz suggested that this bug is fallout for bug 326645, someone might want to look into that
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.
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...
Blocks: 326645
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)
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.
Nominating for 1.8.0.5 because this appears to be a regression from a fix we want (bug 326645)
Assignee: nobody → bzbarsky
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Attached patch Proposed fixSplinter Review
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...
Attachment #225381 - Flags: superreview?(neil)
Attachment #225381 - Flags: review?(bugmail)
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Summary: Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build being different) → [FIX]Crash [@ nsXULDocument::ResumeWalk] during restart (due to last build being different)
Target Milestone: --- → mozilla1.9alpha
Attachment #225381 - Flags: approval-branch-1.8.1?(bugmail)
Comment on attachment 225381 [details] [diff] [review]
Proposed fix

>+            mRootContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::title,
>+                                  title);
Anything wrong with an 80-character line? ;-)
Attachment #225381 - Flags: superreview?(neil) → superreview+
Attachment #225381 - Flags: review?(bugmail)
Attachment #225381 - Flags: review+
Attachment #225381 - Flags: approval-branch-1.8.1?(bugmail)
Attachment #225381 - Flags: approval-branch-1.8.1+
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;
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.
Please land this on trunk/1.8 branches soon for verification
Flags: blocking1.8.0.5? → blocking1.8.0.5+
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...
Attached patch 1.8 branch patchSplinter Review
Attachment #224859 - Attachment is obsolete: true
Fixed on trunk and 1.8 branch.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Keywords: fixed1.8.1
Resolution: --- → FIXED
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.
Attachment #225666 - Flags: approval1.8.0.5?
Comment on attachment 225666 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #225666 - Flags: approval1.8.0.5? → approval1.8.0.5+
Fixed on 1.8.0 branch.
Keywords: fixed1.8.0.5
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Crash Signature: [@ nsXULDocument::ResumeWalk]
You need to log in before you can comment on or make changes to this bug.