417699 - Crash when start SeaMonkey MailNews and Thunderbird [@ nsXULTreeBuilder::SetTree ]

Status: VERIFIED FIXED

Description

[Tobias Fischer]

Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b4pre) Gecko/2008021423 Mnenhy/0.7.5.20005 SeaMonkey/2.0a1pre

SeaMonkey Tinderbox-Builds and also Thunderbird Tinderbox-Builds crash at startup, SM when starting MailNews, even from Browser or with argument -mail.

This regressed between 2008-02-14 12:30:00 and tested SM-Tinderbox-Build 2008021423 and later TB 2008021502 Build.

Because of limited Checkins for MailNews I suspect one of Neils Checkins, might be the one from 12:34 for Bug 258018 or the other at 16:33 for Bug 415601 so I cc' someone.

Well, think this was a Blocker, but feel free to bring it down to major or so.

Comment 1

[Tobias Fischer]

Add some Crash-Reports from 2008021501-regular-Nightly-Build:

98e088ae-dbc7-11dc-9ba8-001a4bd43e5c
9d3ac18a-dbc7-11dc-857a-001a4bd43e5c

SeaMonkey is crashing in "nsXULTreeBuilder::SetTree(nsITreeBoxObject*" but I don't know how to add the relevant Part of the Stack here from Crash-Reporter Site, so only the IDs and one Link:
http://crash-stats.mozilla.com/report/index/98e088ae-dbc7-11dc-9ba8-001a4bd43e5c

Comment 2

[[e-mail]]

Fortunately nothing in your stacks have any relevance to nsUInt32Array.

Naturally my own debug builds don't have any problems...

Comment 3

[Tobias Fischer]

(In reply to comment #2)
> Fortunately nothing in your stacks have any relevance to nsUInt32Array.

Yum, thats right. Have to take a new look in Bonsai, sorry for my fast suspicion. 
> 
> Naturally my own debug builds don't have any problems...
> 
Nice, also I have heard, that someone using WinNT 6.0 was able to run MailNews too from 2008021423-Tinderbox-Build without crashing. 

Regression Range was between 2008-02-14 12:30 and 2008-02-14 23:31:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-14+12%3A30%3A00&maxdate=2008-02-14+23%3A31%3A00&cvsroot=%2Fcvsroot

Comment 4

[Magnus Melin]

Also applies to todays linux build, crashes on startup.

For me running in -safe-mode works though. Tracked it down to lightning (trunk version, had a version from a week back, updated to current build but no help for the crash.)

Comment 5

[Hartmut Figge]

After backing out https://bugzilla.mozilla.org/attachment.cgi?id=303133 of Bug 409111 the crash is gone.

Comment 6

[Hartmut Figge]

(In reply to comment #5)

> After backing out https://bugzilla.mozilla.org/attachment.cgi?id=303133 of Bug
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 409111 the crash is gone.

Should be https://bugzilla.mozilla.org/attachment.cgi?id=303297

Comment 7

[Olli Pettay [:smaug]]

I'm looking at this.

Comment 9

[Olli Pettay [:smaug]]

Created attachment 303545 [details]
testcase, needs "UniversalXPConnect" privileges. Crashes on load.

Seems like the box object change revealed some null pointer crash in
template handling. The testcase shows that similar problem has been there
for a long time.

Comment 10

[Olli Pettay [:smaug]]

Enn, I think most of the cases mRoot is used should check whether it is null.

Comment 11

[Neil Deakin]

Looks like just a null-check in nsTreeBuilder::SetTree is needed. There's already one in Rebuild.

Comment 12

[Olli Pettay [:smaug]]

Yes, that fixes this bug, I hope (just compiling TB), but mRoot is used elsewhere too. See the testcase, for example.

Comment 13

[Olli Pettay [:smaug]]

I can't reproduce this bug locally using Thunderbird :/
But I'll upload a patch, if someone could then test it.

Comment 14

[Olli Pettay [:smaug]]

Created attachment 303551 [details] [diff] [review]
add null checks

Anyone willing to try this?

Comment 15

[Neil Deakin]

 
> NS_IMETHODIMP
> nsXULTemplateBuilder::GetRoot(nsIDOMElement** aResult)
> {
>+    NS_ENSURE_TRUE(mRoot, NS_ERROR_NOT_INITIALIZED);
>     return CallQueryInterface(mRoot, aResult);

GetRoot should just return null, not cause an error.

Comment 16

[Olli Pettay [:smaug]]

Created attachment 303553 [details] [diff] [review]
.root doesn't throw

Comment 17

[Hartmut Figge]

Created attachment 303561 [details]
Result of applying 303553

I have applied attachment 303553 [details] [diff] [review] to a new build of SM 2.0 on linux. No crash, but look at the resulting MailNews-Window. *g*

Scaled down to 800x600

Comment 18

[Olli Pettay [:smaug]]

Created attachment 303567 [details] [diff] [review]
Clear mView

Does this make any difference?
(Would be so great to be able to reproduce this.)

Comment 19

[Hartmut Figge]

Created attachment 303575 [details]
Result of applying 303567

Seems to be OK now.

Comment 20

[Olli Pettay [:smaug]]

Great. I'm now compiling Seamonkey, just to see if I can reproduce the problem
there.
Attachment 303567 [details] [diff] is the right thing to do, IMO.

Is TB or SM moving xul:tree elements in the DOM? So first initializing them
in one place and then moving to a new place. That could lead to the problem, I think

Comment 21

[[e-mail]]

We can do, although not in the default configuration that Hartmut seems to have.

Comment 22

[Olli Pettay [:smaug]]

Can't reproduce on seamonkey either. And I tried to write some testcases, which
remove and add xul:tree back, but those work with or without the patch.
Same thing when setting .style.display to 'none' and back.

Comment 23

[Olli Pettay [:smaug]]

Created attachment 303586 [details] [diff] [review]
v3, clear boxObject when unbinding

I don't like this too much, but should bring back the old behavior and
same behavior as what 1.8 has.
Could anyone test this?

Comment 24

[Hartmut Figge]

Created attachment 303589 [details]
.mozconfig of Hartmut

Mhm, i don't think, my .mozconfig is that unusual. But here it is.

Comment 25

[Hartmut Figge]

(In reply to comment #23)

> Could anyone test this?

Done. Works. No crash.

Comment 26

[Alfred Peters]

I had the Crash on TB (checkout start: Feb 07 18:11:52 PST 2008).
After installing attachment (id=303533) I had the same view as Hartmut.
At this point I tried 'save-mode' (OK I should have done this before) and the view looks fine.
The only Add-on that was installed in my Test-Profile was 'Lightning'. So I deactivate it and the view was fine too!
To check this I got back to the original version: No crash!
Activating 'Lightning': Crash.
Finally I applied attachment (id=303567) and the crash was gone.
HTH

Comment 27

[Olli Pettay [:smaug]]

Comment on attachment 303586 [details] [diff] [review]
v3, clear boxObject when unbinding

Ok, let's take this then. Least-regression-risky.
Adds back those 2 ClearBoxObjectFor calls that were removed in bug 409111. (Similar are there in 1.8). Also adds null checks to template handling to prevent crashes.
Doesn't regress bug 409111 because creating boxobjects is still possible for elements which aren't in a document.
A testcase for this bug would be great.

Comment 28

[Alfred Peters]

(In reply to comment #26)
> I had the Crash on TB (checkout start: Feb 07 18:11:52 PST 2008).
Sorry - That must be (checkout start: Feb 15 10:11:52 PST 2008)

Comment 29

[Hartmut Figge]

(In reply to comment #27)

> A testcase for this bug would be great.

Well, the comment of Alfred showed me, that i should have tested more. At the moment i'm using an SM, which formerly crashed.

This is possible, because i disabled Mnenhy. But SM withou Mnenhy, eeh, your patch is very welcome. ;)

Comment 30

[Olli Pettay [:smaug]]

I meant a minimal testcase.

Comment 31

[Olli Pettay [:smaug]]

Comment on attachment 303586 [details] [diff] [review]
v3, clear boxObject when unbinding

Asking approval to get the regression fixed.

Comment 32

[[e-mail]]

Does this also fix an XPConnect assertion I've been seeing lately?

xpc3250!DEBUG_CheckForComponentsInScope+0x70
xpc3250!XPCWrappedNativeScope::FindInJSObjectScope+0x9e
xpc3250!GetContextFromObject+0x87
xpc3250!nsXPCWrappedJSClass::CallMethod+0x5c
xpc3250!nsXPCWrappedJS::CallMethod+0x3f
xpcom_core!PrepareAndDispatch+0x314
xpcom_core!SharedStub+0x16
gklayout!nsTreeBoxObject::Clear+0x61
gklayout!nsDocument::ClearBoxObjectFor+0x3d
gklayout!nsNodeUtils::LastRelease+0x21c
gklayout!nsGenericElement::Release+0xc8
gklayout!nsXULElement::Release+0xd
xpc3250!XPCJSRuntime::GCCallback+0x547
gklayout!DOMGCCallback+0x1d
js3250!js_GC+0xdf3
js3250!JS_GC+0x48
xul!nsXREDirProvider::DoShutdown+0x154
xul!ScopedXPCOMStartup::~ScopedXPCOMStartup+0x5b
xul!XRE_main+0x321d
seamonkey!NS_internal_main+0x10c
seamonkey!wmain+0x101
seamonkey!wmainCRTStartup+0x12c
kernel32!BaseProcessStart+0x23

Comment 33

[Tobias Fischer]

(In reply to comment #30)
> I meant a minimal testcase.
> 
Humm, sorry that I was not able to prepare a minimal testcase too.

I have noticed, that the crash only occours when Mnenhy (testbuild, you have to ask [e-mail] to get this) with activated "MailNews-Sidebar", or Lightning was installed, so I bring the Bug down to "critical" for now.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 303586 [details] [diff] [review]
v3, clear boxObject when unbinding

a=mconnor on behalf of 1.9 drivers

Comment 35

[Tobias Fischer]

Verified=Fixed 

Have tested with Windows SeaMonkey Tinderbox-Build 2008021900 with Mnenhy installed and MailNews-Sidebar activated and Windows Thunderbird Tinderbox-Build 2008021902 with Lightning installed. No crash. THX.