551739 - possible race on JSContext::requestDepth

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Julian Seward [:jseward]]

There may (or may not) be a race on JSContext::requestDepth.

This is x86_64 Linux, m-c, release build, browser startup.  The two
source locations are

   JS_FRIEND_API(JSContext *)
   js_NextActiveContext(JSRuntime *rt, JSContext *cx)
   {
       JSContext *iter = cx;
   #ifdef JS_THREADSAFE
       while ((cx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) {
           if (cx->requestDepth)  <<----------------------- HERE
               break;
       }
       return cx;
   #else
       return js_ContextIterator(rt, JS_FALSE, &iter);
   #endif
   }
   
   
   JS_PUBLIC_API(void)
   JS_BeginRequest(JSContext *cx)
   {
   #ifdef JS_THREADSAFE
       JS_ASSERT(CURRENT_THREAD_IS_ME(cx->thread));
       if (!cx->requestDepth) {
           JSRuntime *rt = cx->runtime;
           JS_LOCK_GC(rt);
   
           /* Wait until the GC is finished. */
           if (rt->gcThread != cx->thread) {
               while (rt->gcLevel > 0)
                   JS_AWAIT_GC_DONE(rt);
           }
   
           /* Indicate that a request is running. */
           rt->requestCount++;
           cx->requestDepth = 1;
           cx->outstandingRequests++;
           JS_UNLOCK_GC(rt);
           return;
       }
       cx->requestDepth++;  <<--------------------------- HERE
       cx->outstandingRequests++;
   #endif
   }


The Helgrind complaint giving rise to this enquiry is shown below.
Here are some comments from jorendorff on #jsapi:

[19:51] <jorendorff> sewardj: I think the intended scheme here is that 
        requestDepth may only transition from 0 to nonzero within the
        lock; and can only be changed otherwise by cx->thread while a
        request is held (i.e. nonzero to nonzero)

[19:52] <jorendorff> s/within the lock;/within the lock while GC is 
        not happening;/

[19:53] <jorendorff> and readers must either hold the gc lock or must
        be the GC thread

[19:56] <jorendorff> Actually, if the ActiveContext stuff is only
        called during GC, it might be ok (boggle)

[19:59] <jorendorff> Previously I've only thought of four regions,
        "in a request", "holding gcLock", "I am the GC thread", 
        and "none of the above"

[19:59] <jorendorff> and the scheme "can read when in a request or 
        holding gcLock; can read/write when holding gcLock or I am
        the GC thread" is safe.

[20:00] <jorendorff> But demonstrating this is safe requires
        considering a fifth region, "I am going to be the GC thread
        and GC is pending"



Possible data race during read of size 4 at 0x10205358 by thread #5
   at 0x6B1EA7A: js_NextActiveContext(JSRuntime*, JSContext*) (jscntxt.cpp:909)
   by 0x5561A7D: XPCJSRuntime::WatchdogMain(void*) (xpcjsruntime.cpp:812)
   by 0x7679E22: _pt_root (ptthread.c:230)
   by 0x4C2A5D8: mythread_wrapper (hg_intercepts.c:213)
   by 0x4E34A03: start_thread (pthread_create.c:300)
   by 0xAAAA80C: clone (clone.S:112)

 This conflicts with a previous write of size 4 by thread #1
   at 0x6B1044D: JS_BeginRequest (jsapi.cpp:792)
   by 0x5544F0C: nsXPConnect::GetWrapperForObject(JSContext*, JSObject*, JSObject*, nsIPrincipal*, unsigned int, long*) (jsapi.h:569)
   by 0x557ABF0: XPC_WN_JSOp_ThisObject(JSContext*, JSObject*) (xpcwrappednativejsops.cpp:1538)
   by 0x6B56DCF: js_ComputeThis (jsobj.h:424)
   by 0x6B57D9C: js_Invoke (jsinterp.cpp:1218)
   by 0x6B4829F: js_Interpret (jsops.cpp:2277)
   by 0x6B58217: js_Invoke (jsinterp.cpp:1378)
   by 0x6B58787: js_InternalInvoke (jsinterp.cpp:1435)

 Address 0x10205358 is 856 bytes inside a block of size 1624 alloc'd
   at 0x4C24E2C: calloc (vg_replace_malloc.c:467)
   by 0x6B1FFF3: js_NewContext(JSRuntime*, unsigned long) (jsutil.h:191)
   by 0x55E3FDD: mozJSComponentLoader::ReallyInit() (mozJSComponentLoader.cpp:595)
   by 0x55E4E91: mozJSComponentLoader::LoadModule(nsILocalFile*, nsIModule**) (mozJSComponentLoader.cpp:680)
   by 0x610FCAF: nsFactoryEntry::GetFactory(nsIFactory**) (nsComponentManager.cpp:3676)
   by 0x610FE18: nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (nsComponentManager.cpp:1680)
   by 0x610F190: nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (nsComponentManager.cpp:2252)
   by 0x60D4D1B: nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const (nsComponentManagerUtils.cpp:288)
   by 0x60D4067: nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) (nsCOMPtr.cpp:141)
   by 0x5DF083E: nsAppStartupNotifier::Observe(nsISupports*, char const*, unsigned short const*) (nsCOMPtr.h:1031)
   by 0x5538009: XRE_main (nsAppRunner.cpp:3376)
   by 0x400EBB: main (nsBrowserApp.cpp:158)

Comment 1

[Jason Orendorff [:jorendorff]]

Dear everyone: please don't mark this RESO INVALID, not that I think you were going to -- either there's a race, or the rules protecting JSContext::requestDepth are unobvious and not commented (at least, not at its declaration, and not anywhere that I was able to find). Either one is a bug.

Comment 2

[Brendan Eich [:brendan]]

/*
 * Iterate through contexts with active requests. The caller must be holding
 * rt->gcLock in case of a thread-safe build, or otherwise guarantee that the
 * context list is not alternated asynchroniously.
 */
extern JS_FRIEND_API(JSContext *)
js_NextActiveContext(JSRuntime *, JSContext *);

s/alternated/altered/

But of course this doesn't help the unlocked cx->requestDepth++ in JS_BeginRequest. The race to increment from >= 1 to >= 2 is certainly wide open, but JS_BeginRequest does lock on 0 -> 1, and of course JS_EndRequest gets the gcLock going from 1 -> 0. So this seems ok. /me whistles past graveyard...

/be