773998 - Shut down single-browser ("app") content processes when the browser closes

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Followup from bug 762802.  I wrote the somewhat trivial patch and lost the lottery, crashed :(.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hey guys, I'm seeing a crash during XPCOM shutdown in content processes

(gdb) call PrintJSStack()
$1 = 0x1e6b6f0 "0 fa_observe() [\"chrome://browser/content/forms.js\":152]\n    this = [object Object]\n1 <TOP LEVEL> [\"<unknown>\":0]\n    <failed to get 'this' value>\n"

this code is

        Services.obs.removeObserver(this, "ime-enabled-state-changed", false);

crashing at

    aObject->GetDomain(getter_AddRefs(objectURI));

(gdb) f 5
#5  0x00007f1b501b1305 in nsScriptSecurityManager::CheckSameOriginPrincipal (aSubject=0x1948fc0, aObject=0x0) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:958
(gdb) p aObject
$2 = (nsIPrincipal *) 0x0

Here's the backtrace.

#4  <signal handler called>
#5  0x00007f1b501b1305 in nsScriptSecurityManager::CheckSameOriginPrincipal (aSubject=0x1948fc0, aObject=0x0) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:958
#6  0x00007f1b501b5f4e in nsScriptSecurityManager::doGetObjectPrincipal (aObj=0x7f1b1a358140) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2319
#7  0x00007f1b501b5b07 in nsScriptSecurityManager::GetFunctionObjectPrincipal (cx=0x18b45d0, obj=0x7f1b1a358140, fp=0x7f1b362bd0b8, rv=0x7fff70c5aa74) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2191
#8  0x00007f1b501b5be1 in nsScriptSecurityManager::GetFramePrincipal (this=0x18b3cc0, cx=0x18b45d0, fp=0x7f1b362bd0b8, rv=0x7fff70c5aa74) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2214
#9  0x00007f1b501b63ac in nsScriptSecurityManager::IsCapabilityEnabled (this=0x18b3cc0, capability=0x7f1b51d55c5e "UniversalXPConnect", result=0x7fff70c5aafe) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2452
#10 0x00007f1b501b7317 in nsScriptSecurityManager::CheckXPCPermissions (this=0x18b3cc0, cx=0x18b45d0, aObj=0x1889730, aJSObject=0x0, aSubjectPrincipal=0x0, aObjectSecurityLevel=0x0) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2818
#11 0x00007f1b501b6cc2 in nsScriptSecurityManager::CanCreateWrapper (this=0x18b3cc0, cx=0x18b45d0, aIID=..., aObj=0x1889730, aClassInfo=0x0, aPolicy=0x0) at /home/cjones/mozilla/inbound/caps/src/nsScriptSecurityManager.cpp:2684
#12 0x00007f1b505277ca in XPCWrappedNative::InitTearOff (this=0x1c09030, ccx=..., aTearOff=0x1a5e6a0, aInterface=0x1a04370, needJSObject=0) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedNative.cpp:2134
#13 0x00007f1b50527254 in XPCWrappedNative::FindTearOff (this=0x1c09030, ccx=..., aInterface=0x1a04370, needJSObject=0, pError=0x7fff70c5b02c) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedNative.cpp:1974
#14 0x00007f1b504cf6d8 in XPCCallContext::CanCallNow (this=0x7fff70c5b240) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCCallContext.cpp:258
#15 0x00007f1b50527e62 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedNative.cpp:2335
#16 0x00007f1b5053542d in XPC_WN_CallMethod (cx=0x18b45d0, argc=3, vp=0x7f1b362bd138) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
#17 0x00007f1b516f2453 in js::CallJSNative (cx=0x18b45d0, native=0x7f1b505351d2 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/cjones/mozilla/inbound/js/src/jscntxtinlines.h:382
#18 0x00007f1b516f9de8 in js::InvokeKernel (cx=0x18b45d0, args=..., construct=js::NO_CONSTRUCT) at /home/cjones/mozilla/inbound/js/src/jsinterp.cpp:344
#19 0x00007f1b51706ec1 in js::Interpret (cx=0x18b45d0, entryFrame=0x7f1b362bd0b8, interpMode=js::JSINTERP_NORMAL) at /home/cjones/mozilla/inbound/js/src/jsinterp.cpp:2442
#20 0x00007f1b516f99a3 in js::RunScript (cx=0x18b45d0, script=0x7f1b359df670, fp=0x7f1b362bd0b8) at /home/cjones/mozilla/inbound/js/src/jsinterp.cpp:301
#21 0x00007f1b516f9e93 in js::InvokeKernel (cx=0x18b45d0, args=..., construct=js::NO_CONSTRUCT) at /home/cjones/mozilla/inbound/js/src/jsinterp.cpp:355
#22 0x00007f1b5164ac64 in js::Invoke (cx=0x18b45d0, args=..., construct=js::NO_CONSTRUCT) at /home/cjones/mozilla/inbound/js/src/jsinterp.h:119
#23 0x00007f1b516fa055 in js::Invoke (cx=0x18b45d0, thisv=..., fval=..., argc=3, argv=0x7fff70c5ce20, rval=0x7fff70c5cb20) at /home/cjones/mozilla/inbound/js/src/jsinterp.cpp:387
#24 0x00007f1b5163c30b in JS_CallFunctionValue (cx=0x18b45d0, obj=0x7f1b1a358040, fval=..., argc=3, argv=0x7fff70c5ce20, rval=0x7fff70c5cb20) at /home/cjones/mozilla/inbound/js/src/jsapi.cpp:5555
#25 0x00007f1b5051d1d5 in nsXPCWrappedJSClass::CallMethod (this=0x199fdd0, wrapper=0x1d828f0, methodIndex=3, info=0x16e5440, nativeParams=0x7fff70c5cf50) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
#26 0x00007f1b50513e3f in nsXPCWrappedJS::CallMethod (this=0x1d828f0, methodIndex=3, info=0x16e5440, params=0x7fff70c5cf50) at /home/cjones/mozilla/inbound/js/xpconnect/src/XPCWrappedJS.cpp:580
#27 0x00007f1b50e6b7ad in PrepareAndDispatch (self=0x1d81c80, methodIndex=3, args=0x7fff70c5d0d0, gpregs=0x7fff70c5d050, fpregs=0x7fff70c5d080) at /home/cjones/mozilla/inbound/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
#28 0x00007f1b50e6a96f in SharedStub () from /home/cjones/mozilla/2gaia-dbg/dist/bin/libxul.so
#29 0x00007f1b50dfc2d3 in nsObserverList::NotifyObservers (this=0x1b28258, aSubject=0x1676c18, aTopic=0x7f1b51f5f21c "xpcom-shutdown", someData=0x0) at /home/cjones/mozilla/inbound/xpcom/ds/nsObserverList.cpp:99
#30 0x00007f1b50dfddb5 in nsObserverService::NotifyObservers (this=0x1889730, aSubject=0x1676c18, aTopic=0x7f1b51f5f21c "xpcom-shutdown", someData=0x0) at /home/cjones/mozilla/inbound/xpcom/ds/nsObserverService.cpp:149
#31 0x00007f1b50de48fd in mozilla::ShutdownXPCOM (servMgr=0x0) at /home/cjones/mozilla/inbound/xpcom/build/nsXPComInit.cpp:581
#32 0x00007f1b50de46f4 in NS_ShutdownXPCOM_P (servMgr=0x0) at /home/cjones/mozilla/inbound/xpcom/build/nsXPComInit.cpp:541
#33 0x00007f1b4f3cb2a8 in XRE_TermEmbedding () at /home/cjones/mozilla/inbound/toolkit/xre/nsEmbedFunctions.cpp:194
#34 0x00007f1b50b572ea in mozilla::ipc::ScopedXREEmbed::Stop (this=0x15c8a98) at /home/cjones/mozilla/inbound/ipc/glue/ScopedXREEmbed.cpp:94
#35 0x00007f1b50b01c10 in mozilla::dom::ContentProcess::CleanUp (this=0x15c8750) at /home/cjones/mozilla/inbound/dom/ipc/ContentProcess.cpp:31


Halp!  I have no idea what to do here :).  Will post a rotten band-aid patch.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Null check this doohickey

I have no idea what this patch does.  Please advice.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Shut down app processes when the last browser closes

Other than that minor hiccup, it really was that easy!

Comment 4

[Boris Zbarsky [:bzbarsky]]

So at first glance that looks like old_doGetObjectPrincipal is returning null, right?

This can happen if:

1)  The object is a parent-less function.  Bobby, is his kosher?
2)  The object is a function parented to a Call and js::GetObjectParentMaybeScope on the
    Call returns null.  When is that? 
3)  We manage to walk all the way up the parent chain before finding an XPConnect object. 
    Again, when would this happen?

Bobby?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642282 [details] [diff] [review]
Null check this doohickey

r=me, I guess, since if it's null it's hard to say anything about it here.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> 1)  The object is a parent-less function.  Bobby, is his kosher?

I don't think so.

> 2)  The object is a function parented to a Call and
> js::GetObjectParentMaybeScope on the
>     Call returns null.  When is that? 

I'd think never, but maybe luke would know better?

> 3)  We manage to walk all the way up the parent chain before finding an
> XPConnect object. 
>     Again, when would this happen?

It shouldn't. The parent chain should lead to the global, and that should always be an XPConnect object.

cjones, can you break with gdb and tell us why that function is returning null (assuming that's the case)?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Might be easier for you to debug locally; building b2g for desktop is quite easy, https://wiki.mozilla.org/Gaia/Hacking#Building_B2G .

STR are
 (1) Apply patch here
 (2) Unlock lock screen (pin is 0000)
 (3) Tap calculator icon to launch calculator app
 (4) Hold down HOME key to bring up task manager
 (5) Click the calculator task and swipe upwards to close it

Then, BOOM.

I'll see if I can answer your specific question later today.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can only extremely infrequently reproduce this bug now; I think it's something latent, unrelated to this patch.  Spinning off the work to bug 774606.

I couldn't easily figure out why old_doGetObjectPrincipal() was returning null, without understanding the code or having a reverse debugger.  (It's a rather complicated function.)

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 642282 [details] [diff] [review]
Null check this doohickey

It looks like something bad is indeed happening elsewhere.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jlebar, r? --> you ;).

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 642283 [details] [diff] [review]
Shut down app processes when the last browser closes

>+ContentParent::ShutDown()

I should r+ this without reading the rest because you used "shut down" instead of "shutdown" as the verb.  Yay.

>@@ -411,16 +432,29 @@ ContentParent::ActorDestroy(ActorDestroy
> }
> 
> TabParent*
> ContentParent::CreateTab(PRUint32 aChromeFlags, bool aIsBrowserFrame)
> {
>   return static_cast<TabParent*>(SendPBrowserConstructor(aChromeFlags, aIsBrowserFrame));
> }
> 
>+void
>+ContentParent::NotifyTabDestroyed(PBrowserParent* aTab)
>+{
>+    // There can be more than one PBrowser for a given app process
>+    // because of popup windows.  When the last one closes, shut
>+    // us down.
>+    if (IsForApp() && ManagedPBrowserParent().Length() == 1) {
>+        MessageLoop::current()->PostTask(
>+            FROM_HERE,
>+            NewRunnableMethod(this, &ContentParent::ShutDown));

I've never seen this MessageLoop business; why do you need to do this instead of using the Gecko message loop?

Aside from the MessageLoop trick, this was surprisingly straightforward.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #11)
> Comment on attachment 642283 [details] [diff] [review]
> Shut down app processes when the last browser closes
> 
> >+void
> >+ContentParent::NotifyTabDestroyed(PBrowserParent* aTab)
> >+{
> >+    // There can be more than one PBrowser for a given app process
> >+    // because of popup windows.  When the last one closes, shut
> >+    // us down.
> >+    if (IsForApp() && ManagedPBrowserParent().Length() == 1) {
> >+        MessageLoop::current()->PostTask(
> >+            FROM_HERE,
> >+            NewRunnableMethod(this, &ContentParent::ShutDown));
> 
> I've never seen this MessageLoop business; why do you need to do this
> instead of using the Gecko message loop?

There's no specific reason, it's mostly tradition in the IPC code.  MessageLoop is the chromium event-loop stuff, and it tends to be a little easier to use / has more goodies, and doesn't require creating threadsafe-refcounted tasks unless necessary.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d3f276e0ac

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a1d3f276e0ac