311332 - Installing extensions by putting them in the Extensions folder crashes Firefox/Thunderbird at startup [@ nsXBLBinding::AllowScripts]

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Henrik Gemal]

One of the new ways of installing extensions is by dropping them in the root of
the extension folder. then the app at startup asks the user if he wants to
install the extension.
this works just fine on firefox but crashes thunderbird. you get the install
extension just fine but when you press ok and the extension get installed and
thunderbird tries to start you crash.

to reproduce:
download an thunderbird extension that you dont have installed. place the xpi in
the extensions folder. start thunderbird = crash

this is only for nightly build (and I think 1.5 builds)

talkback:
TB10286332G

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Possibly a regression from bug 292591, CCing Boris.

Comment 2

[timeless]

Incident ID: 10286332
Stack Signature	nsXBLBinding::AllowScripts 39eda2ab
Product ID	ThunderbirdTrunk
Build ID	2005100506
Trigger Time	2005-10-06 04:14:51.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	THUNDE~1.EXE + (001e76a5)
URL visited	
User Comments	
Since Last Crash	13 sec
Total Uptime	13 sec
Trigger Reason	Access violation
Source File, Line No.
e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 1233
Stack Trace 	
nsXBLBinding::AllowScripts 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 1233]
nsXBLBinding::ExecuteDetachedHandler 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 795]
nsGlobalWindow::HandleDOMEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp,
line 1514]
DocumentViewerImpl::PageHide 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/layout/base/nsDocumentViewer.cpp,
line 1215]
nsDocShell::FirePageHideNotification 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/docshell/base/nsDocShell.cpp,
line 890]
nsDocShell::Destroy 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/docshell/base/nsDocShell.cpp,
line 3511]
nsXULWindow::Destroy 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/xpfe/appshell/src/nsXULWindow.cpp,
line 511]
nsWebShellWindow::Destroy 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp,
line 850]
nsAppShellService::Observe 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 550]
NS_ShutdownXPCOM_P 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/xpcom/build/nsXPComInit.cpp,
line 795]
ScopedXPCOMStartup::~ScopedXPCOMStartup 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/toolkit/xre/nsAppRunner.cpp,
line 553]
main 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mail/app/nsMailApp.cpp,
line 62]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Interesting.  So what's happening here is that we're shutting down because we
registered a new extension.  This destroyes our webshell window off of an XPCOM
shutdown notification.  Which destroys a docshell, which fires a PageHide event.
 Which calls nsXBLBinding::ExecuteDetachedHandler (via
nsBindingManager::ExecuteDetachedHandlers), which calls AllowScripts. Now
AllowScripts gets the security manager from nsContentUtils and uses it... and
crashes, because it's null.  Because the layout module as a whole has already
shut down before any of this happened.

Compare bug 308328.

Note that the patch for bug 292591 did null-check the security manager and just
disallowed script execution if there wasn't one, which is why this is only
crashing on trunk.

So possible solutions:

1)  Go back to null-checking the security manager.
2)  Don't run detached handlers after layout module shutdown.
3)  Bail early out of nsGlobalWindow::HandleDOMEvent after layout module
    shutdown.
4)  Some combination of the above.

Comment 4

[Scott MacGregor]

this looks like it could be a dupe of Bug 306959. 

Comment 5

[Benjamin Smedberg]

*** Bug 306959 has been marked as a duplicate of this bug. ***

Comment 6

[timeless]

*** Bug 311685 has been marked as a duplicate of this bug. ***

Comment 7

[Peter Van der Beken [:peterv]]

This was fixed as part of bug 308328, but I'm leaving this open to decide
whether we want to implement one of the other solutions mentioned in comment 3.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think #2 is the way to go there....  Or rather, I think at layout module
shutdown we should run any remaining detached handlers and be done with it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

sicking, thoughts?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

2 or 3 sounds like the way to go here. Once layout has shut down we should do as
little as possible since otherwise we'll never be able to rely on layout having
started.

What sounds strange to me is that the XPCOM notification to shut down happens
after layout is shut down? Shouldn't that be the other way around, i.e. send out
notifications first and close down modules after that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er..  Layout watches xpcom-shutdown.  When that happens it shuts itself down. 
Unfortunately other modules watch it too, and might get it after layout, and
might access layout objects after layout has shut down as a result...

It's sort of a fundamental problem with our startup/shutdown stuff -- neither
guarantees ordering.

Comment 12

[Adam Guthrie]

*** Bug 312434 has been marked as a duplicate of this bug. ***

Comment 13

[Mike Cowperthwaite]

(In reply to comment #7)
> This was fixed as part of bug 308328

Maybe.  Bug 306959, which was duped to this, still manifests with TB 1.6a1-1015.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Then it's probably not a dup, since this bug _is_ fixed.

Comment 15

[Henrik Gemal]

this is NOT fixed. Still crashes both Firefox and Thunderbird if there's a XPI
file in the extension folder at startup.
TBIRD: TB10886325Z
FFOX: TB10886012K

Comment 16

[Ria Klaassen (not reading all bugmail)]

Bug 311685 (Firefox crashes when running a trunk build after a branch build in
the same profile) is still present.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> this is NOT fixed.

How so?

> TBIRD: TB10886325Z
> FFOX: TB10886012K

Different stack (you DID look at these before commenting on this bug, yes?),
hence different crash bug.  Bug 306959, in fact.

Comment 18

[Boris Zbarsky [:bzbarsky]]

*** Bug 313262 has been marked as a duplicate of this bug. ***

Comment 19

[:Gavin Sharp [email: [e-mail]]]

*** Bug 314161 has been marked as a duplicate of this bug. ***

Comment 20

[Adam Guthrie]

*** Bug 320323 has been marked as a duplicate of this bug. ***

Comment 21

[Boris Zbarsky [:bzbarsky]]

Once bug 331117 is fixed we can probably remove the null-check here.

Comment 22

[Mike Cowperthwaite]

(In reply to comment #21)
> Once bug 331117 is fixed we can probably remove the null-check here.

OK, that's happened.  Is that null-check the reason this bug is still open?

Comment 23

[Wayne Mery (:wsmwk)]

Mike in comment #22:
> (In reply to comment #21)
> > Once bug 331117 is fixed we can probably remove the null-check here.
> 
> OK, that's happened.  Is that null-check the reason this bug is still open?

seems so.

Comment 24

[Nochum Sossonko [:Natch]]

Resolving as FIXED, the null-check can be removed in another bug. Revert if I'm wrong...

Comment 25

[Wayne Mery (:wsmwk)]

(In reply to comment #24)
> Resolving as FIXED, the null-check can be removed in another bug. Revert if I'm
> wrong...

Unless the "remove null-check" bug has already been filed or dealt with here as follow up to  Bug 292591 -  content XBL scripts run even when Javascript turned off (Thunderbird exposed to any JS exploit) - it seems to me this should remain open

reopening

Comment 26

[Jesse Ruderman]

--> bug 516289