531303 - Instantiating a JS component from a background thread crashes (JS_ASSERT)

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

The component in question implements nsICertOverrideService. PSM uses that component from a background thread. JS doesn't like that:

Assertion failure: CURRENT_THREAD_IS_ME(cx->thread), at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/jsapi.cpp:928

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x43806950 (LWP 25757)]
0x00007ffff3ac7095 in raise () from /lib/libc.so.6
(gdb) bt
#0  0x00007ffff3ac7095 in raise () from /lib/libc.so.6
#1  0x00007ffff3ac8af0 in abort () from /lib/libc.so.6
#2  0x00007ffff76f5409 in JS_Assert (s=0x7ffff770f938 "CURRENT_THREAD_IS_ME(cx->thread)", 
    file=0x7ffff770f2f8 "/usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/jsapi.cpp", ln=928)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/jsutil.cpp:69
#3  0x00007ffff75fa826 in JS_BeginRequest (cx=0x7fffeb6ae800) at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/jsapi.cpp:928
#4  0x00007fffeaf0099a in JSCLContextHelper (this=0x43805160, loader=0x7fffefc92260)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1686
#5  0x00007fffeaf03ba0 in mozJSComponentLoader::GlobalForLocation (this=0x7fffefc92260, aComponent=0x7fffdb6b49c0, aGlobal=0x7fffdb4e4188, 
    aLocation=0x7fffdb4e4190, exception=0x0) at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1110
#6  0x00007fffeaf05167 in mozJSComponentLoader::LoadModule (this=0x7fffefc92260, aComponentFile=0x7fffdb6b49c0, aResult=0x43805750)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/js/src/xpconnect/loader/mozJSComponentLoader.cpp:695
#7  0x00007ffff7105fb6 in nsFactoryEntry::GetFactory (this=0x7fffeb648e10, aFactory=0x438057c8)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/xpcom/components/nsComponentManager.cpp:3691
#8  0x00007ffff71062af in nsComponentManagerImpl::CreateInstanceByContractID (this=0x7fffefcec160, 
    aContractID=0x7fffddebc150 "@mozilla.org/security/certoverride;1", aDelegate=0x0, aIID=..., aResult=0x43805860)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/xpcom/components/nsComponentManager.cpp:1680
#9  0x00007ffff7108587 in nsComponentManagerImpl::GetServiceByContractID (this=0x7fffefcec160, aContractID=0x7fffddebc150 "@mozilla.org/security/certoverride;1", 
    aIID=..., result=0x43805978) at /usr/local/google/home/cbiesinger/mozilla-1.9.2/xpcom/components/nsComponentManager.cpp:2252
#10 0x00007ffff7095458 in CallGetService (aContractID=0x7fffddebc150 "@mozilla.org/security/certoverride;1", aIID=..., aResult=0x43805978)
    at nsComponentManagerUtils.cpp:94
#11 0x00007ffff70954ec in nsGetServiceByContractID::operator() (this=0x43805960, aIID=..., aInstancePtr=0x43805978) at nsComponentManagerUtils.cpp:278
#12 0x00007fffdde53b2b in nsCOMPtr<nsICertOverrideService>::assign_from_gs_contractid (this=0x43805b70, gs=..., aIID=...)
    at ../../../../dist/include/nsCOMPtr.h:1229
#13 0x00007fffdde53b85 in nsCOMPtr (this=0x43805b70, gs=...) at ../../../../dist/include/nsCOMPtr.h:604
#14 0x00007fffdde5d335 in nsNSSBadCertHandler (arg=0x7fffdb575300, sslSocket=0x7fffdb4b1430)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/security/manager/ssl/src/nsNSSIOLayer.cpp:3316
#15 0x00007fffdd99dcf0 in ssl3_HandleCertificate (ss=0x7fffdb4cd000, b=0x7fffdb4d535a "\016", length=0) at ssl3con.c:7287
#16 0x00007fffdd99f594 in ssl3_HandleHandshakeMessage (ss=0x7fffdb4cd000, b=0x7fffdb4d504e "", length=780) at ssl3con.c:7959
#17 0x00007fffdd99f9dd in ssl3_HandleHandshake (ss=0x7fffdb4cd000, origBuf=0x7fffdb4cd350) at ssl3con.c:8083
#18 0x00007fffdd9a0470 in ssl3_HandleRecord (ss=0x7fffdb4cd000, cText=0x43805ef0, databuf=0x7fffdb4cd350) at ssl3con.c:8346
#19 0x00007fffdd9a1567 in ssl3_GatherCompleteHandshake (ss=0x7fffdb4cd000, flags=0) at ssl3gthr.c:206
#20 0x00007fffdd9a407b in ssl_GatherRecord1stHandshake (ss=0x7fffdb4cd000) at sslcon.c:1258
#21 0x00007fffdd9af33f in ssl_Do1stHandshake (ss=0x7fffdb4cd000) at sslsecur.c:151
#22 0x00007fffdd9b154b in ssl_SecureSend (ss=0x7fffdb4cd000, 
    buf=0x7fffdb404000 "GET / HTTP/1.1\r\nHost: mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2b4pre) Gecko/20091119 Namoroka/3.6b4pre\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,"..., len=552, flags=0) at sslsecur.c:1176
#23 0x00007fffdd9b16e6 in ssl_SecureWrite (ss=0x7fffdb4cd000, 
    buf=0x7fffdb404000 "GET / HTTP/1.1\r\nHost: mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2b4pre) Gecko/20091119 Namoroka/3.6b4pre\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,"..., len=552) at sslsecur.c:1221
#24 0x00007fffdd9b865b in ssl_Write (fd=0x7fffdb4b1430, buf=0x7fffdb404000, len=552) at sslsock.c:1488
#25 0x00007fffdde37c49 in nsSSLThread::Run (this=0x7fffde147c00) at /usr/local/google/home/cbiesinger/mozilla-1.9.2/security/manager/ssl/src/nsSSLThread.cpp:1045
#26 0x00007fffdde36ef0 in nsPSMBackgroundThread::nsThreadRunner (arg=0x7fffde147c00)
    at /usr/local/google/home/cbiesinger/mozilla-1.9.2/security/manager/ssl/src/nsPSMBackgroundThread.cpp:44
#27 0x00007ffff6a2ca0c in _pt_root (arg=0x7fffdedbecf0) at /usr/local/google/home/cbiesinger/mozilla-1.9.2/nsprpub/pr/src/pthreads/ptthread.c:228
#28 0x00007ffff7bca3f7 in start_thread () from /lib/libpthread.so.0
#29 0x00007ffff3b6cb4d in clone () from /lib/libc.so.6
#30 0x0000000000000000 in ?? ()

Tested on mozilla-1.9.2, as of changeset 773ba9060398

Comment 1

[timeless]

i think i have a queue element for this ;-)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Here's a component that shows the problem. Just put it in your components directory, update components.list, and from the second time you start firefox you'll crash when going to a site with a cert warning.
http://code.google.com/p/selenium/source/browse/webdriver/trunk/firefox/src/extension/components/badCertListener.js

Comment 3

[timeless]

Attachment: draft

Comment 4

[timeless]

Please note that your component is vaguely buggy. If it wants to be threadsafe (and the contract in question is obviously expected to be so), then it must declare nsIClassInfo.flags & nsIClassInfo.THREADSAFE

The fact that we're not enforcing this is covered by another set of patches I haven't finished.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Thanks, the patch fixes the crash for me on 1.9.2.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Is there any reason that we can't just use the per-thread safe context here?

Comment 7

[timeless]

sorry, i'm sleepy, what?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

The underlying bug here is that we end up using the same context on two different threads without calling JS_ClearContextThread. Your patch fixes this by using a context per module loaded, which ensures that each context is only ever used on one thread (since it is created and destroyed by a stack-based object).

I'm wondering if, instead of creating a new context for each component, we could just use the safe context (one per thread).

Comment 9

[timeless]

sure, i get the idea, which api would give me that?

Comment 10

[timeless]

nm nsIThreadJSContextStack.safeJSContext

Comment 11

[timeless]

Comment on attachment 414840 [details] [diff] [review]
draft

I've lost way too much time on this. There's no JS_GetThreadStackLimit, which means i can't steal the safe critter, squat on it, and restore it when i'm done.

If you want something better, i'll gladly review your patch. otherwise we're being mean to people who need this and are crashing. delaying code for years for some idealism is not the way we should be doing work. it results in work not getting done.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 414840 [details] [diff] [review]
draft

While I agree that standing on principal is no reason to stop progress, overdue haste is no reason to push incomplete or otherwise incorrect patches into the tree. That being said, my question about using the safe context for this was just a question and since you've answered that it isn't sufficient here, we should move forward.

diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
@@ -536,7 +544,6 @@ WriteScriptToStream(JSContext *cx, JSScr
 
 mozJSComponentLoader::mozJSComponentLoader()
     : mRuntime(nsnull),
-      mContext(nsnull),


Why do this? It seems like it's asking for mContext to be accidentally used uninitialized.

@@ -587,38 +604,15 @@ mozJSComponentLoader::ReallyInit()
         NS_FAILED(rv = mRuntimeService->GetRuntime(&mRuntime)))
         return rv;
 
+    // This context is for use by UnloadModules
+    mContext = JS_NewContext(mozJSComponentLoader::sSelf->mRuntime, 256);
+    if (!mContext)
+        return NS_ERROR_OUT_OF_MEMORY;

This isn't currently true -- but perhaps it will be. If we decide that it isn't important to GC from UnloadModules, then can mContext go away?

@@ -1405,9 +1403,7 @@ mozJSComponentLoader::UnloadModules()
+    // XXX We want to force a GC.

JS_GC(mContext);?

@@ -1679,14 +1677,45 @@ mozJSComponentLoader::Observe(nsISupport
 //----------------------------------------------------------------------
 
 JSCLContextHelper::JSCLContextHelper(mozJSComponentLoader *loader)
+    return;

Nit: I don't see a reason  for this return; statement.

r=mrbkap with those comment addressed.

Comment 13

[timeless]

Attachment: patch

Comment 14

[Phil Ringnalda (:philor)]

try tested and approved

Comment 15

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/9b18eada106e

Comment 16

[Phil Ringnalda (:philor)]

Backed out: apparently I confused myself about which things were and weren't happy (or maybe it's very freakily intermittent about failing): on the actual tinderbox, every xpcshell test run failed, crashing in test_bug408412.js.

Assortment of logs:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272685967.1272687409.31199.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272682797.1272684149.24418.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272681686.1272685874.28007.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272682285.1272683933.23903.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272682721.1272687100.30619.gz

Comment 17

[Johnny Stenback (:jst)]

Not blocking 2.0 on this, but this blocks fennec-2.0. timeless, can you help sort out what went wrong when this landed the first time?

Comment 18

[timeless]

according to tinderbox logs i hit bug 571619

neil: is that correct or merely a coincidence that the tinderbox comment for that timestamp matches the rough timestamp for the links to the tinderbox logs.

Comment 19

[Phil Ringnalda (:philor)]

That is the comment for every single log which no longer exists (and we only keep logs for 30 or 40 days, depending on what you believe, so those logs have been gone for about 9 months) - it's not coincidence, just some bug in Tinderbox and in the notes data file.

Has anyone tried pushing it to try again? Possibly try actually did show the problem, possibly it does now whether or not it did then, possibly the problem goes away with a clobber (and thus would never be seen on try), possibly the problem has gone away since. In any case, the first two steps for someone who wants to see it land would be to push it to try, and to build with it and run xpcshell locally.

Comment 20

[Phil Ringnalda (:philor)]

Something like 40 changes in mozJSComponentLoader.cpp since last April, the patch is pretty thoroughly rotted.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

We won't seem to get this in for Fennec 4, and I am working on other ways to fix bug 618919.

Comment 22

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 23

[Boris Zbarsky [:bzbarsky]]

We don't support non-mainthread JS components.