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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
P1
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
crash, fixed1.8.0.5, fixed1.8.1, regression
Points:
---
Bug Flags:
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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".
(Reporter)

Comment 1

11 years ago
Created attachment 224841 [details]
stack trace (mac debug)
(Reporter)

Comment 2

11 years ago
> 3. Launch trunk build from the command line.

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

Comment 3

11 years ago
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)

Updated

11 years ago

Comment 4

11 years ago
Created attachment 224859 [details] [diff] [review]
this should do it
Attachment #224859 - Flags: superreview?(neil)
Attachment #224859 - Flags: review?(neil)

Comment 5

11 years ago
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)

Comment 6

11 years ago
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

11 years ago
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

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

Ben? Bryner? Neil?

Comment 9

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

Comment 10

11 years ago
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?
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...
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 16

11 years ago
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 17

11 years ago
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...
Created attachment 225666 [details] [diff] [review]
1.8 branch patch
Attachment #224859 - Attachment is obsolete: true
Fixed on trunk and 1.8 branch.
Status: NEW → RESOLVED
Last Resolved: 11 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

Updated

9 years ago
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.