Closed Bug 531303 Opened 11 years ago Closed 3 years ago

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

Categories

(Core :: XPConnect, defect)

1.9.2 Branch
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
blocking2.0 --- -

People

(Reporter: Biesinger, Assigned: timeless)

Details

(Whiteboard: [needs an unrotted patch pushed to try])

Attachments

(1 file, 1 obsolete file)

8.38 KB, patch
timeless
: review+
Details | Diff | Splinter Review
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
i think i have a queue element for this ;-)
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
Attached patch draft (obsolete) — Splinter Review
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.
Thanks, the patch fixes the crash for me on 1.9.2.
Attachment #414840 - Flags: review?(mrbkap)
Is there any reason that we can't just use the per-thread safe context here?
sorry, i'm sleepy, what?
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).
sure, i get the idea, which api would give me that?
nm nsIThreadJSContextStack.safeJSContext
Attachment #414840 - Flags: review?(mrbkap)
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.
Attachment #414840 - Flags: review?(mrbkap)
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.
Attachment #414840 - Flags: review?(mrbkap) → review+
Attached patch patchSplinter Review
Assignee: nobody → timeless
Attachment #414840 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #433043 - Flags: review+
try tested and approved
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9b18eada106e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.3a5 → ---
Whiteboard: [needs non-crashing patch]
blocking2.0: --- → ?
tracking-fennec: --- → 2.0+
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?
blocking2.0: ? → -
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.
Whiteboard: [needs non-crashing patch] → [needs another try run]
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.
Something like 40 changes in mozJSComponentLoader.cpp since last April, the patch is pretty thoroughly rotted.
Whiteboard: [needs another try run] → [needs an unrotted patch pushed to try]
We won't seem to get this in for Fennec 4, and I am working on other ways to fix bug 618919.
No longer blocks: 618919
tracking-fennec: 2.0+ → ---
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago3 years ago
Resolution: --- → INACTIVE
We don't support non-mainthread JS components.
Resolution: INACTIVE → WONTFIX
You need to log in before you can comment on or make changes to this bug.