244106 - Crash on shutdown attempting to unload JS component.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

I have a JS component implementing a service. As XPCOM is shut down there is a
crash due to the fact that the safe js context held by the appshell service
cannot be properly cleared. This is because appshellsvc's tries to free the safe
js context in its dtor which is called as services are being freed, which means
the attempt to get the context stack service will fail, and the safe js context
will not be freed properly, and all hell will break loose when we try to unload
js components. 

We need to move the freeing of the safe js context into an XPCOM shutdown
observer so that the call to do_GetService will succeed and the safe js context
be successfully removed.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix

Add xpcom-shutdown observer to safely destroy safe js context, remove bogus
comment about the appshell service being owned strongly by observer service,
add an assertion in the state where the thread js context stack cannot be
obtained.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

I reworded the assertion to "XPConnect ContextStack gone before XPCOM shutdown?"
at brendan's suggestion. 

Comment 3

[Darin Fisher]

the patch looks good to me fwiw.

  (i'd just change NS_ASSERTION(0,...) to NS_ERROR)

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 148895 [details] [diff] [review]
patch to fix

sr=me, with the assertion change mentioned in person.

/be

Comment 5

[Brendan Eich [:brendan]]

Yeah, NS_ERROR(...), not NS_ASSERTION(0, ...).

So this bug should stay open, because we want XPConnect to stop requiring such a
fragile song-and-dance from the embedding app that it requires via the
SafeContext interface methods.  I think we want an nsIXPCScriptContext,
implemented by nsJSContext, possibly even the superclass of nsIScriptContext
(comments from jst welcome).

/be

Comment 6

[Brendan Eich [:brendan]]

XPConnect guys, see comment 5 in particular.

/be

Comment 7

[Brendan Eich [:brendan]]

In comment 0, s/freed/cleared/g in

>js context in its dtor which is called as services are being freed, which means
>the attempt to get the context stack service will fail, and the safe js context
>will not be freed properly, and all hell will break loose when we try to unload
>js components. 

/be

Comment 8

[Johnny Stenback (:jst)]

(In reply to comment #5)
> So this bug should stay open, because we want XPConnect to stop requiring such a
> fragile song-and-dance from the embedding app that it requires via the
> SafeContext interface methods.  I think we want an nsIXPCScriptContext,
> implemented by nsJSContext, possibly even the superclass of nsIScriptContext
> (comments from jst welcome).

Yeah, an nsIXPCScriptContent that would be inherited by nsIScriptContext (which
we might want to rename to nsIJSContext or whatever since it's really JS
specific, and will remain it). nsIXPCScriptContext probably doesn't really need
much in terms of methods, a getter for the native JSContext, but probably not
much else...

Comment 9

[Dan M]

Comment on attachment 148895 [details] [diff] [review]
patch to fix

Kill the hidden window and JS safe context earlier, when services are still
guaranteed. Well, OK. But doesn't this put a constraint on other services,
particularly JS services, not to rely on their context even as early as during
the XPCOM shutdown notification? In fact I vaguely recall nixing such a change
years back because of that concern. But if it helps today...

PS please don't set mDeleteCalled to TRUE in the Observe method. At the very
least change its name. But I believe you can just get rid of that member
variable entirely, since its purpose is to prevent  the hidden window from
trying to access AppShellService as it's being deleted within the ASS dtor.
That safety check is no longer necessary with this patch.

Comment 10

[Brendan Eich [:brendan]]

Danm: lxr sez the only test of mDeleteCalled is in
nsAppShellService::UnregisterTopLevelWindow, and the only call to that method is
from
http://lxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsXULWindow.cpp#439, and
the r.v. is ignored.  Still, does it make sense to suppress UTLW calls during
XPCOM shutdown?

It's true we need a better model for JS components.  But the old model was just
busted, for a long time (maybe forever): services are freed before components
are unloaded, so there's no way a JS component could outlive the app-shell
service.  Worse, the code as it stood called do_GetService from a service impl
dtor, which was bound to fail, leaving the safe context pointer in XPConnect
dangling at garbage.

/be

Comment 11

[Dan M]

As long as you figure this helps more than it hurts. I suspect it'll harm some
little-known JS service somewhere, but I can buy into this being an improvement.

Under The Live Wire? no...
Until This Lily Wilts? no...
Unbosom to The Lincoln Wannabe? no...
Unravels The Lixiviated Wimple? no...

mDeleteCalled should no longer be triggered. It'll certainly be a misnomer.

Comment 12

[Brendan Eich [:brendan]]

Unregister Top Level Window, prosaic but what I meant.  What's it good fer, if'n
it ain't running at service shutdown today?  Why run it a bit earlier in XPCOM
shutdown, when the "xpcom-shutdown" topic is observed?

/be

Comment 13

[Dan M]

>mDeleteCalled should no longer be triggered.

Well that's not true. mDeleteCalled will be triggered, and it'll stop Hidden
Window from being unregistered. I don't think it'll hurt anything, but it seems
pointless. Unpatched, it prevents a problem similar to the one you're solving;
an attempt to reinstate the ASS from within its destructor. Keep it if you like,
but change its name to mImDoingTheDishes or mIHearYouKnocking. Anything but Sue.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

bsmedberg landed this on the trunk last week, I just now changed "mDeleteCalled"
to "mXPCOMShuttingDown" to be more accurate. 

Fixed br & trunk.