395707 - Crash @nsWindowSH::GetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*) during EM restart

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a8pre) Gecko/2007091004 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a8pre) Gecko/2007091004 Minefield/3.0a8pre

On nightly build, i reproduced a top #14 crash (according to breakpad), when applying a nightly software update, with an incompatible extension loaded.

Reproducible: Always

Steps to Reproduce:
1. install an incompatible extension on nightly (eg. Nightly test tool 1.3b1)
2. Open a prior nightly build, and do a Help > Updates
3. When update window appears, do the nightly update
4. When dialog asks to restore session of open sites, click restart
5. Verify Firefox updates software, and then upon restartng Firefox, it crashes at nsWindowSH::GetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*)
Actual Results:  
Crash @nsWindowSH::GetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*) upon restarting session after software update

Expected Results:  
no crash.

Comment 1

[Dave Townsend [:mossop]]

Breakpad seems to show a pretty clear regression on 1st September nightly:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&maxdate=1188560137&mindate=1188647880

Comment 2

[Dave Townsend [:mossop]]

The crash appears to be occurring during the xpcom-shutdown phase of the EM requested restart.

Comment 3

[Martijn Wargers (dead)]

According to Dave, bug 395897 might be about the same issue (that bug has a testcase).

Comment 4

[Mike Connor [:mconnor]]

I think we need this for b1..

Comment 5

[Robert Sayre]

I can also reproduce this by running Firefox2 with an incompatible extension (e.g. Firebug 1.0.5) and then starting Minefield. 

Comment 6

[Dave Townsend [:mossop]]

This particular case occurs when attempting to call nonBrowserWindowShutdown on the hidden window. It is just the function call that crashes, even if the function is empty.

Comment 7

[Dave Townsend [:mossop]]

So the regression range is kind of right, this has started crashing because nonBrowserWindowShutdown was added in bug 342600.

Comment 8

[Dave Townsend [:mossop]]

During XPCOM shutdown there is a last ditch attempt to destroy any remaining hidden window:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/appshell/src/nsAppShellService.cpp&rev=1.235&mark=561#552

Unfortunately this comes after nsDOMScriptObjectFactory sees the xpcom-shutdown and destroys nsDOMClassInfo, pretty much breaking any scripts that would try to run in the hidden window.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/src/base/nsDOMScriptObjectFactory.cpp&rev=1.20&mark=289#258

On a normal shutdown the hidden window is destroyed in advance of the xpcom shutdown so there isn't an issue. On the EM initiated restart however the hidden window is not destroyed.

Comment 9

[Dave Townsend [:mossop]]

Oops, this was blocking-firefox3+ so should be blocking1.9

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This simply destroys the hidden window before xpcom shutdown when a restart has been forced

Comment 11

[Benjamin Smedberg]

Comment on attachment 284949 [details] [diff] [review]
patch rev 1

As hacks go, this one kinda sucks, but I'll let it slide because I can't think of something better right now.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> (From update of attachment 284949 [details] [diff] [review])
> As hacks go, this one kinda sucks, but I'll let it slide because I can't think
> of something better right now.

About the only thoughts I came up with were either to destroy the hidden window directly from NS_ShutdownXPCOM always (not a good idea I think), or add a new observer topic send from NS_ShutdownXPCOM say xpcom-before-shutdown or something which the appshell service could listen to and destroy the hidden window there.

Both of those seem pretty invasive than this.

Comment 13

[Dave Townsend [:mossop]]

Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.198; previous revision: 1.197
done

Comment 15

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102304 Minefield/3.0a9pre.  Repro steps in comment #1 no longer happen.

Crash Reporter at: http://crash-stats.mozilla.com/report/list?range_unit=weeks&branch=1.9&range_value=2&signature=nsWindowSH%3A%3AGetProperty(nsIXPConnectWrappedNative*%2C+JSContext*%2C+JSObject*%2C+long%2C+long*%2C+int*), also shows no further instances of this crash after Build ID:  2007102000, which is the next build after Dave's checkin from Comment #13.  Marking bug as verified.