Closed Bug 1559659 Opened 6 years ago Closed 6 years ago

XPCOM threads running on a JS Helperthread pool need to hold an initialized JSContext until shutdown

Categories

(Core :: XPConnect, enhancement, P3)

Product:
Core
Component:
XPConnect
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: KrisWright, Assigned: KrisWright)

References

Details

Attachments

(3 files)

Bug 1559659 - 1. Liberate TlsContext.set() process from JSContext ctor, add thread id ptr, make it possible to clear thread context & id without destroying JSContext
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559659 - 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState
47 bytes, text/x-phabricator-request
jandem
: feedback+
Details | Review
Bug 1559659 - 3. Get rid of per-thread jscontext instantiation and use the global pool instead
47 bytes, text/x-phabricator-request
Details | Review

Some helper thread tasks (primarily ParseTasks) use JSContext extensively while completing jobs. Native Spidermonkey helperthreads facilitate this by holding a JSContext for the duration of their thread loop[1]. We've already addressed the need for HelperThread() null checks[2] so that offthread contexts will work whether or not they have stored a native HelperThread pointer value. I was able to complete tasks by creating a new JSContext for the scope of the RunnableTask::runTask() method itself, but while creating and initializing a new JSContext for every offthread task works in practice it's not the best approach to fix the actual problem.

I'm not completely sure at the moment what the best approach will be for this since JS doesn't know about XPCOM and the new thread pool doesn't have a "complete" view of JSContext. From what I understand there will need to be calls made between the two to make a thread hold a JSContext for its lifetime, so that TlsContext.get() returns an actual JSContext for offthread tasks.

[1] https://searchfox.org/mozilla-central/rev/8c09b0389ff4e6263a3f10b1dbd2b6145d7a2aa7/js/src/vm/HelperThreads.cpp#2492
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1558556

NI'ing a few folks with more knowledge on TLSContext. I've played with a few ways to do this by passing a jscontext through a few layers of callbacks and I guess in theory it's possible, but from what I'm aware nsthread can't initialize the jscontext on its own from its scope so there's a bit of back and forth. I played with lazily instantiating a JSContext on a per-task basis (check for TlsContext.get()==null during runTask() on offthread tasks that need it, create one then) but it's fairly hacky so I'm curious if there's a better solution. How should I be going about this?

Flags: needinfo?(jdemooij)
Flags: needinfo?(jcoppeard)

I think your options are:

  1. Continue creating a JSContext per thread somehow. If the thread pool does not offer a run-once init method you'll have to do this lazily; it sounds like this is what you did.

  2. Rewrite the HelperThread code to use an array or Vector of JSContexts (>= N contexts for N threads) and have threads take a JSContext from that (with some RAII class to set/reset TlsContext). Question here is whether those helper thread JSContexts depend on always being tied to a particular thread - I don't think so, at this point, and else it's probably fixable. If we do this we should also use this mechanism for the current HelperThread framework to not have XPCOM/browser-only bugs.

Flags: needinfo?(jdemooij)

Thinking about it more, I prefer option 2: especially once we have a unified thread pool across Gecko, it would be nice to not have threads refer to JS contexts (via TLSContext) when they are idle or running non-JS tasks. The global list of available contexts would just be to avoid constructing/destroying a JSContext per task, as JSContext is unfortunately pretty heavyweight.

(In reply to Jan de Mooij [:jandem] from comment #3)
Sounds good to me.

Flags: needinfo?(jcoppeard)

For the long term solution, something suggested by memshrink folks was that we are likely to have some sort of virtual threads when we do the general thread pool. This would mean be similar to option 2 proposed here but generalized for various related issues that other components may experience.

Depends on: 1562309

(In reply to Jan de Mooij [:jandem] from comment #3)

Thinking about it more, I prefer option 2: especially once we have a unified thread pool across Gecko, it would be nice to not have threads refer to JS contexts (via TLSContext) when they are idle or running non-JS tasks. The global list of available contexts would just be to avoid constructing/destroying a JSContext per task, as JSContext is unfortunately pretty heavyweight.

I've had the chance to play with this and the only problem is how JSContexts make the assumption that they will be operating on their own thread for their entire lifetime. It looks like the only thread-specific assumption JSContext makes now is that it keeps ThreadData types, so I filed bug 1562309 to address that first per conversations yesterday with jonco. Do you know if there are any other threading assumptions that JSContext makes, or is that all that's left?

(In reply to Kristen Wright :KrisWright from comment #6)

Do you know if there are any other threading assumptions that JSContext makes, or is that all that's left?

Another thing (I should have thought of this before) is stack limits for over-recursion checks (used at least by parse tasks). Here you'd need to do something like: call GetNativeStackBase() to set cx->nativeStackBase and then call JS_SetNativeStackQuota. The current stack size/quota for helper threads is defined here: [1]. We want to make sure the thread pool's threads have a similar stack size.

It might make sense to hack your way through this to see what the issues are: comment out the ThreadData assertions, failing over-recursion checks, etc.

[0] https://searchfox.org/mozilla-central/rev/29f8e6aebd2b2660b9c76858ccb0c6eaf35dd7aa/js/public/RootingAPI.h#912
[1] https://searchfox.org/mozilla-central/rev/29f8e6aebd2b2660b9c76858ccb0c6eaf35dd7aa/js/src/vm/HelperThreads.cpp#1082-1083

Priority: -- → P3

Created SetThread/ClearThread functions to handle JSContext/tid setting/clearing & updated existing implementation of JSContext to reflect. CurrentThread is ContextData because JSContext should already have a "claim" on the thread if it's clearing these.

Created a vector of JSContext* (gHelperThreadContexts, which could be named better), initialized at creation of GlobalHelperThreadState, destroyed when GlobalHelperThreadState.finish() is called. Note that this implementation makes the assumption that the creation and destruction of these objects always happens in the same order (helper thread state created -> main thread context created -> main thread context destroyed -> helper thread state destroyed). In this scenario context checks can clear by each context claiming the main thread during its ctor/dtor but this is only possible if the main thread context is not set at the time.

Just curious, I imagined this Vector would live in GlobalHelperThreadState and would reuse the locking we have for that (AutoLockHelperThreadState). Is there a reason that doesn't work?

(In reply to Jan de Mooij [:jandem] from comment #10)

Just curious, I imagined this Vector would live in GlobalHelperThreadState and would reuse the locking we have for that (AutoLockHelperThreadState). Is there a reason that doesn't work?

Ah, I was mostly thinking about XPCOM. Now that you mention it, that would work as long as we keep GlobalHelperThreadState active when thread jobs are diverted to XPCOM (maybe just avoid creating its own internal threads if the xpconnect pool exists once that's going on). The process of getting cx and the like should be the same so I'll refactor it that way.

Assignee: nobody → kwright

Got rid of the per-thread JSContext created at the start of each thread. Tasks that require JSContext (ParseTasks, IonBuilder, Wasm tier 2 generators, GCParallel) now request an unused context to set to their thread. Tasks which do not use JSContext will not request one.

(In reply to Jan de Mooij [:jandem] from comment #7)

Another thing (I should have thought of this before) is stack limits for over-recursion checks (used at least by parse tasks). Here you'd need to do something like: call GetNativeStackBase() to set cx->nativeStackBase and then call JS_SetNativeStackQuota. The current stack size/quota for helper threads is defined here: [1]. We want to make sure the thread pool's threads have a similar stack size.

It might make sense to hack your way through this to see what the issues are: comment out the ThreadData assertions, failing over-recursion checks, etc.

[0] https://searchfox.org/mozilla-central/rev/29f8e6aebd2b2660b9c76858ccb0c6eaf35dd7aa/js/public/RootingAPI.h#912
[1] https://searchfox.org/mozilla-central/rev/29f8e6aebd2b2660b9c76858ccb0c6eaf35dd7aa/js/src/vm/HelperThreads.cpp#1082-1083

I think I've got this addressed now. Initially nativeStackBase set itself with the main thread on creation which causes over-recursions in practice so I changed it so that it isn't a constant anymore and set it when I set the context to a thread. I want to be certain that's safe to do with NativeStackBase. It looks like HelperThread worries about its actual stack size independently of cx.

Also, would the maybeFreeUnusedMemory routine that HelperThread would use in its main loop be useful to the global context pool? I could make it something the GlobalHelperThreadState is responsible for, and when contexts are freed from use (such as from finishing a task that uses a context) they could check to see if they need to free the tempLifoAlloc.

(In reply to Kristen Wright :KrisWright from comment #13)

I want to be certain that's safe to do with NativeStackBase. It looks like HelperThread worries about its actual stack size independently of cx.

Resetting the stack base + using JS_SetNativeStackQuota with a quota that's, say, 70-80% of the actual stack size (as the current code is doing) should be fine, but the question is what the actual stack size is for XPCOM's threads, right? Can we pass or get that somewhere?

Also, would the maybeFreeUnusedMemory routine that HelperThread would use in its main loop be useful to the global context pool? I could make it something the GlobalHelperThreadState is responsible for, and when contexts are freed from use (such as from finishing a task that uses a context) they could check to see if they need to free the tempLifoAlloc.

That sounds great. One thing I noticed is that we call maybeFreeUnusedMemory while holding the HelperThreadState lock, we should probably stop doing that while you're at it (freeing memory can be surprisingly slow and there's no need to hold the lock for that).

edit: there's the HelperThread::shouldFreeUnusedMemory flag that does require the lock. Maybe we should just unlock before we do the freeAll().

(In reply to Jan de Mooij [:jandem] from comment #14)

Resetting the stack base + using JS_SetNativeStackQuota with a quota that's, say, 70-80% of the actual stack size (as the current code is doing) should be fine, but the question is what the actual stack size is for XPCOM's threads, right? Can we pass or get that somewhere?

nsThread is able to init with a stack size, so from my understanding of other thread work going on there's already plans to define threads of certain stack sizes for certain jobs. The good news for Spidermonkey is that XPCOM pools can spin up threads with the same stack size as HelperThread. The XPCJSThreadPool could address that before creating threads for Spidermonkey. I filed bug 1565302 to make sure the thread pool uses the correct stack size.

edit: there's the HelperThread::shouldFreeUnusedMemory flag that does require the lock. Maybe we should just unlock before we do the freeAll().

I figured that since jscontext will have to address this itself anyway in XPCOM, I should just make freeing unused memory on the helper thread a responsibility of the context being used for a task. That way jscontext can freeAll before clearing the context.

Comment on attachment 9075833 [details]
Bug 1559659 - 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState

This has taken a bit more to make happen than I had thought it would, so before I finish it up I want to make sure this is what you had in mind.

Attachment #9075833 - Flags: feedback?(jdemooij)

Comment on attachment 9075833 [details]
Bug 1559659 - 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState

LGTM! I didn't look at it in detail, but I like the design and posted some nits on Phabricator.

Attachment #9075833 - Flags: feedback?(jdemooij) → feedback+
Pushed by kwright@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4501af908d40 1. Liberate TlsContext.set() process from JSContext ctor, add thread id ptr, make it possible to clear thread context & id without destroying JSContext r=jandem https://hg.mozilla.org/integration/autoland/rev/6a862204faf4 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState r=jandem https://hg.mozilla.org/integration/autoland/rev/9f68b578bdfc 3. Get rid of per-thread jscontext instantiation and use the global pool instead r=jandem

Failure logs: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256990244&repo=autoland&lineNumber=2586
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256994111&repo=autoland&lineNumber=3360

So I'm not sure why we are hitting this segfault, and it's hard for me to reproduce since it's on android and I'm not getting the same issue on other platforms. I've tried a few things - checking to see if the vector exists, ensuring cx is not a nullptr, destroying the list in a different place - but I'm still getting a segfault on these specific tests in GlobalHelperThreadState::DestroyHelperContexts(). Do you know if there are any key differences to allocation or to GlobalHelperThreadState in android that would make the context list unable to be destroyed?

Flags: needinfo?(kwright) → needinfo?(jdemooij)

(In reply to Kris Wright :KrisWright from comment #20)

Do you know if there are any key differences to allocation or to GlobalHelperThreadState in android that would make the context list unable to be destroyed?

The thing that's failing is this assertion (about TlsContext being not set): https://hg.mozilla.org/integration/autoland/file/9f68b578bdfcb96f8b3508a254a5f6e2aa582d87/js/src/vm/HelperThreads.cpp#l1235

What's happening is probably that we're leaking a JSRuntime on shutdown, the case warned about here: https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/js/src/vm/Initialization.cpp#198

Try removing the assertion or change to a MOZ_ASSERT_IF(!JSRuntime::hasLiveRuntimes(), ...) and see if that helps on Try?

Flags: needinfo?(jdemooij)

(In reply to Jan de Mooij [:jandem] from comment #21)

The thing that's failing is this assertion (about TlsContext being not set): https://hg.mozilla.org/integration/autoland/file/9f68b578bdfcb96f8b3508a254a5f6e2aa582d87/js/src/vm/HelperThreads.cpp#l1235

What's happening is probably that we're leaking a JSRuntime on shutdown, the case warned about here: https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/js/src/vm/Initialization.cpp#198

Try removing the assertion or change to a MOZ_ASSERT_IF(!JSRuntime::hasLiveRuntimes(), ...) and see if that helps on Try?

Thank you for the help! I realize now that the assert there was redundant because SetThread() asserts anyway.

Pushed by kwright@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fddd762b06a7 1. Liberate TlsContext.set() process from JSContext ctor, add thread id ptr, make it possible to clear thread context & id without destroying JSContext r=jandem https://hg.mozilla.org/integration/autoland/rev/8dc0ee208fcc 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState r=jandem https://hg.mozilla.org/integration/autoland/rev/40803e58b850 3. Get rid of per-thread jscontext instantiation and use the global pool instead r=jandem

Backed out for SM bustages on wasm/jsapi/table/*.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/66752cc173bbf080fbb468628f5472ff8d19a9b3
Log link: https://taskcluster-artifacts.net/DCJ3il4dT3KYyShCPved3w/0/public/logs/live_backing.log
There was also a perma tier 2 failure - browser_dbg_rr_breakpoints-01.js | Uncaught exception - at resource://devtools/shared/protocol/Front.js:69 - Error: Connection closed, pending request to server0.conn0.child1/thread19, type frames failed
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257973463&repo=autoland&lineNumber=954

Log snippet for the SM:

[task 2019-07-23T22:53:33.683Z] WARNING: ThreadSanitizer: data race (pid=19223)
[task 2019-07-23T22:53:33.683Z] Write of size 8 at 0x7b7400001090 by thread T7:
[task 2019-07-23T22:53:33.683Z] #0 js::LifoAlloc::releaseAll() /builds/worker/workspace/build/src/js/src/ds/LifoAlloc.h:780:22 (js+0x3fa925)
[task 2019-07-23T22:53:33.683Z] #1 ~AutoSetHelperThreadContext /builds/worker/workspace/build/src/js/src/vm/HelperThreads.h:708:25 (js+0x3f4930)
[task 2019-07-23T22:53:33.683Z] #2 js::GCParallelTask::runFromHelperThread(js::AutoLockHelperThreadState&) /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:1778 (js+0x3f4930)
[task 2019-07-23T22:53:33.683Z] #3 js::HelperThread::handleGCParallelWorkload(js::AutoLockHelperThreadState&) /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:1797:21 (js+0x3f4b88)
[task 2019-07-23T22:53:33.683Z] #4 js::HelperThread::threadLoop() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:2562:5 (js+0x3f644f)
[task 2019-07-23T22:53:33.683Z] #5 js::HelperThread::ThreadMain(void*) /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:2082:36 (js+0x3f1485)
[task 2019-07-23T22:53:33.683Z] #6 callMain<0> /builds/worker/workspace/build/src/js/src/threading/Thread.h:239:5 (js+0x3fed5d)
[task 2019-07-23T22:53:33.683Z] #7 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start(void*) /builds/worker/workspace/build/src/js/src/threading/Thread.h:232 (js+0x3fed5d)
[task 2019-07-23T22:53:33.683Z]
[task 2019-07-23T22:53:33.683Z] Location is heap block of size 2184 at 0x7b7400000a00 allocated by main thread:
[task 2019-07-23T22:53:33.683Z] #0 malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:672:5 (js+0x14cc77)
[task 2019-07-23T22:53:33.683Z] #1 malloc /builds/worker/workspace/build/src/memory/build/malloc_decls.h:38:1 (js+0x1ff229)
[task 2019-07-23T22:53:33.683Z] #2 moz_arena_malloc /builds/worker/workspace/build/src/memory/build/malloc_decls.h:38 (js+0x1ff229)
[task 2019-07-23T22:53:33.683Z] #3 moz_arena_malloc /builds/worker/workspace/build/src/memory/build/malloc_decls.h:116 (js+0x1ff229)
[task 2019-07-23T22:53:33.683Z] #4 js_arena_malloc /builds/worker/workspace/build/src/obj-spider/dist/include/js/Utility.h:392:10 (js+0x3ea341)
[task 2019-07-23T22:53:33.683Z] #5 js_malloc /builds/worker/workspace/build/src/obj-spider/dist/include/js/Utility.h:396 (js+0x3ea341)
[task 2019-07-23T22:53:33.683Z] #6 js_new<JSContext, nullptr_t, JS::ContextOptions> /builds/worker/workspace/build/src/obj-spider/dist/include/js/Utility.h:545 (js+0x3ea341)
[task 2019-07-23T22:53:33.683Z] #7 MakeUnique<JSContext, nullptr_t, JS::ContextOptions> /builds/worker/workspace/build/src/obj-spider/dist/include/js/UniquePtr.h:43 (js+0x3ea341)
[task 2019-07-23T22:53:33.683Z] #8 js::GlobalHelperThreadState::initializeHelperContexts() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:1208 (js+0x3ea341)
[task 2019-07-23T22:53:33.683Z] #9 js::CreateHelperThreadsState() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:73:27 (js+0x3e9e45)
[task 2019-07-23T22:53:33.683Z] #10 JS::detail::InitWithFailureDiagnostic(bool) /builds/worker/workspace/build/src/js/src/vm/Initialization.cpp:175:3 (js+0x4010a0)
[task 2019-07-23T22:53:33.683Z] #11 JS_InitWithFailureDiagnostic /builds/worker/workspace/build/src/obj-spider/dist/include/js/Initialization.h:78:10 (js+0x1beb79)
[task 2019-07-23T22:53:33.683Z] #12 main /builds/worker/workspace/build/src/js/src/shell/js.cpp:10872 (js+0x1beb79)
[task 2019-07-23T22:53:33.683Z]
[task 2019-07-23T22:53:33.683Z] Thread T7 'JS Helper' (tid=19268, running) created by main thread at:
[task 2019-07-23T22:53:33.683Z] #0 pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:975:3 (js+0x14e165)
[task 2019-07-23T22:53:33.683Z] #1 js::Thread::create(void* ()(void), void*) /builds/worker/workspace/build/src/js/src/threading/posix/Thread.cpp:97:7 (js+0x82b106)
[task 2019-07-23T22:53:33.683Z] #2 init<void (&)(void ), js::HelperThread > /builds/worker/workspace/build/src/js/src/threading/Thread.h:124:12 (js+0x3eaa9c)
[task 2019-07-23T22:53:33.683Z] #3 js::GlobalHelperThreadState::ensureInitialized() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:1146 (js+0x3eaa9c)
[task 2019-07-23T22:53:33.683Z] #4 js::EnsureHelperThreadsInitialized() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:89:30 (js+0x3ea756)
[task 2019-07-23T22:53:33.683Z] #5 JSRuntime::init(JSContext, unsigned int, unsigned int) /builds/worker/workspace/build/src/js/src/vm/Runtime.cpp:201:32 (js+0x51b3bc)
[task 2019-07-23T22:53:33.683Z] #6 js::NewContext(unsigned int, unsigned int, JSRuntime) /builds/worker/workspace/build/src/js/src/vm/JSContext.cpp:160:17 (js+0x4304ef)
[task 2019-07-23T22:53:33.683Z] #7 JS_NewContext(unsigned int, unsigned int, JSRuntime*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:377:10 (js+0x7a70e7)
[task 2019-07-23T22:53:33.683Z] #8 main /builds/worker/workspace/build/src/js/src/shell/js.cpp:11257:25 (js+0x1c5c24)
[task 2019-07-23T22:53:33.683Z]
[task 2019-07-23T22:53:33.683Z] Thread T6 'JS Helper' (tid=19267, running) created by main thread at:
[task 2019-07-23T22:53:33.683Z] #0 pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:975:3 (js+0x14e165)
[task 2019-07-23T22:53:33.683Z] #1 js::Thread::create(void* ()(void), void*) /builds/worker/workspace/build/src/js/src/threading/posix/Thread.cpp:97:7 (js+0x82b106)
[task 2019-07-23T22:53:33.683Z] #2 init<void (&)(void ), js::HelperThread > /builds/worker/workspace/build/src/js/src/threading/Thread.h:124:12 (js+0x3eaa9c)
[task 2019-07-23T22:53:33.683Z] #3 js::GlobalHelperThreadState::ensureInitialized() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:1146 (js+0x3eaa9c)
[task 2019-07-23T22:53:33.683Z] #4 js::EnsureHelperThreadsInitialized() /builds/worker/workspace/build/src/js/src/vm/HelperThreads.cpp:89:30 (js+0x3ea756)
[task 2019-07-23T22:53:33.683Z] #5 JSRuntime::init(JSContext, unsigned int, unsigned int) /builds/worker/workspace/build/src/js/src/vm/Runtime.cpp:201:32 (js+0x51b3bc)
[task 2019-07-23T22:53:33.683Z] #6 js::NewContext(unsigned int, unsigned int, JSRuntime) /builds/worker/workspace/build/src/js/src/vm/JSContext.cpp:160:17 (js+0x4304ef)
[task 2019-07-23T22:53:33.683Z] #7 JS_NewContext(unsigned int, unsigned int, JSRuntime*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:377:10 (js+0x7a70e7)
[task 2019-07-23T22:53:33.683Z] #8 main /builds/worker/workspace/build/src/js/src/shell/js.cpp:11257:25 (js+0x1c5c24)
[task 2019-07-23T22:53:33.683Z]
[task 2019-07-23T22:53:33.683Z] SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/build/src/js/src/ds/LifoAlloc.h:780:22 in js::LifoAlloc::releaseAll()
[task 2019-07-23T22:53:33.683Z] ==================
[task 2019-07-23T22:53:33.683Z] ThreadSanitizer: reported 7 warnings
[task 2019-07-23T22:53:33.683Z] TEST-UNEXPECTED-FAIL | wasm/jsapi/table/get-set.any.js | (args: "") [1.5 s]
[task 2019-07-23T22:53:33.966Z] Makefile:64: recipe for target 'check-jstests' failed
[task 2019-07-23T22:53:33.966Z] make[1]: *** [check-jstests] Error 1
[task 2019-07-23T22:53:33.966Z] make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src'
[task 2019-07-23T22:53:33.966Z] Makefile:296: recipe for target 'check-jstests' failed
[task 2019-07-23T22:53:33.966Z] make: *** [check-jstests] Error 2
[task 2019-07-23T22:53:33.973Z] + BUILD_STATUS=2
[task 2019-07-23T22:53:33.973Z] + mkdir -p /builds/worker/artifacts/
[task 2019-07-23T22:53:33.974Z] + cp -rL /builds/worker/workspace/build/src/obj-spider/dist/bin/js /builds/worker/workspace/build/src/obj-spider/dist/bin/jsapi-tests /builds/worker/workspace/build/src/obj-spider/dist/bin/js-gdb.py /builds/worker/artifacts/
[task 2019-07-23T22:53:34.144Z] + gzip -c /builds/worker/workspace/clang/bin/llvm-symbolizer
[task 2019-07-23T22:53:34.340Z] + exit 2
[taskcluster 2019-07-23 22:53:34.869Z] === Task Finished ===
[taskcluster 2019-07-23 22:53:42.438Z] Unsuccessful task run with exit code: 2 completed in 4771.087 seconds

Flags: needinfo?(kwright)

(In reply to Narcis Beleuzu [:NarcisB] from comment #24)

Backed out for SM bustages on wasm/jsapi/table/*.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/66752cc173bbf080fbb468628f5472ff8d19a9b3
Log link: https://taskcluster-artifacts.net/DCJ3il4dT3KYyShCPved3w/0/public/logs/live_backing.log
There was also a perma tier 2 failure - browser_dbg_rr_breakpoints-01.js | Uncaught exception - at resource://devtools/shared/protocol/Front.js:69 - Error: Connection closed, pending request to server0.conn0.child1/thread19, type frames failed
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257973463&repo=autoland&lineNumber=954

Well, that explains why freeing the unused memory was done while the lock was held. Not sure if/how the fail in browser_dbg_rr_breakpoints-01.js can be caused by the patch though.

Flags: needinfo?(kwright)

(In reply to Kris Wright :KrisWright from comment #25)

Well, that explains why freeing the unused memory was done while the lock was held.

It seems to me the underlying problem is that we don't lock when we call clearThread() (and the context's threadId is not an Atomic either). We could rename setThread/clearThread to setHelperThread/clearHelperThread and make them take an AutoLockHelperThreadState& argument to enforce this. (AutoLockHelperThreadState can be forward-declared in JSContext.h if necessary.) What do you think?

Not sure if/how the fail in browser_dbg_rr_breakpoints-01.js can be caused by the patch though.

It probably has to do with mozilla::Atomic changes and WebReplay records those. This is a tier 2 job and Brian can fix this after you land this.

(In reply to Jan de Mooij [:jandem] from comment #26)

We could rename setThread/clearThread to setHelperThread/clearHelperThread and make them take an AutoLockHelperThreadState& argument to enforce this.

Oh and same for the context-available method. Then all the "helper context management" methods ensure proper locking.

(In reply to Jan de Mooij [:jandem] from comment #26)

It seems to me the underlying problem is that we don't lock when we call clearThread() (and the context's threadId is not an Atomic either). We could rename setThread/clearThread to setHelperThread/clearHelperThread and make them take an AutoLockHelperThreadState& argument to enforce this. (AutoLockHelperThreadState can be forward-declared in JSContext.h if necessary.) What do you think?

The only thing that I can see being a bit weird is when we set the main thread context. Maybe the main thread should have its own set method, or even handle that during JSContext::init() after checking that the new ContextKind is main-thread?

(In reply to Kris Wright :KrisWright from comment #28)

or even handle that during JSContext::init() after checking that the new ContextKind is main-thread?

Makes a lot of sense I think, to have JSContext itself handle this transparently for the main-thread case.

(In reply to Jan de Mooij [:jandem] from comment #29)

Makes a lot of sense I think, to have JSContext itself handle this transparently for the main-thread case.

So it looks like it requires we call JSContext::init(...) before we init the runtime (from here https://searchfox.org/mozilla-central/source/js/src/vm/JSContext.cpp#166) so that the runtime can use TlsContext. Does JSContext::init need the runtime to be initialized first? It seems like it should work but I'm not sure if the ordering was intentional.

(In reply to Kris Wright :KrisWright from comment #30)

So it looks like it requires we call JSContext::init(...) before we init the runtime (from here https://searchfox.org/mozilla-central/source/js/src/vm/JSContext.cpp#166) so that the runtime can use TlsContext. Does JSContext::init need the runtime to be initialized first? It seems like it should work but I'm not sure if the ordering was intentional.

If everything is green if you do that, it's probably fine (I don't think the ordering was intentional).

See also: Bug 1568410

There is some thread shutdown behaviour that is supposed to happen to prevent lsan from complaining later. See https://searchfox.org/mozilla-central/rev/0bcdfa99339c0512e3730903835fb6f8ed45e3c4/js/src/threading/Mutex.cpp#15-38 for the TLS stuff.

Pushed by kwright@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa9186737abd 1. Liberate TlsContext.set() process from JSContext ctor, add thread id ptr, make it possible to clear thread context & id without destroying JSContext r=jandem https://hg.mozilla.org/integration/autoland/rev/f4b6a279a331 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState r=jandem https://hg.mozilla.org/integration/autoland/rev/1d509e451876 3. Get rid of per-thread jscontext instantiation and use the global pool instead r=jandem

(In reply to Narcis Beleuzu [:NarcisB] from comment #34)

Backed out for SM bustages on Runtime.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/6d7591cdc9118062826b03a316b5a48c472db5f2
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258516922&repo=autoland&lineNumber=14426

I had a bit of an oversight that you don't have to call destroyRuntime() unless you've called runtime->init(...) first - an uninitialized runtime will assert. I guess I'll need to correct that!

Flags: needinfo?(kwright)
Pushed by kwright@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/632548616c12 1. Liberate TlsContext.set() process from JSContext ctor, add thread id ptr, make it possible to clear thread context & id without destroying JSContext r=jandem https://hg.mozilla.org/integration/autoland/rev/a56b3fe80336 2. Create, initialize, destroy vector of JSContext* with GlobalHelperThreadState r=jandem https://hg.mozilla.org/integration/autoland/rev/0eff8ea5dbf1 3. Get rid of per-thread jscontext instantiation and use the global pool instead r=jandem
Blocks: 1569799
Regressions: 1569805
Blocks: 1569819
Regressions: 1569835
Regressions: 1570883
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: