676376 - prevent multi-threaded JSRuntime access in new web worker memory reporters

Status: RESOLVED FIXED

Description

[Luke Wagner [:luke]]

These were landed recently and block bug 650411 which wants to assert that all access to a JSRuntime comes from a single owning thread.

Fear not, bent has a plan!

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Created attachment 551272 [details] [diff] [review]
Patch, v1

Here's the best option I think. I went with this approach because we're about to split workers even further (for shared workers) and we can't touch those runtimes on the main thread any longer.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 551272 [details] [diff] [review]
Patch, v1

Though would love a second r+ on this from luke.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 551272 [details] [diff] [review]
Patch, v1

Looks good to me.  I confirmed that this does indeed fix the assert I was seeing previously.  Thanks!

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/65a834be2590

Comment 5

[:Ehsan Akhgari]

http://hg.mozilla.org/mozilla-central/rev/65a834be2590

Comment 6

[Bobby Holley (:bholley)]

So, I'm kind of confused here. Over in bug 716167, I assert that GetXPConnect is never called off-main-thread. This works everywhere except in toolkit/components/aboutmemory/tests/test_aboutmemory.xul, where I get crashes with the following stack:

CrashInJS () at js/src/jsutil.cpp:95
JS_Assert at /files/mozilla/repos/a/js/src/jsutil.cpp:115
nsXPConnect::GetXPConnect at js/xpconnect/src/nsXPConnect.cpp:173
nsXPConnect::GetRuntimeInstance at js/xpconnect/src/nsXPConnect.cpp:287
mozilla::xpconnect::memory::CollectCompartmentStatsForRuntime at js/xpconnect/src/XPCJSRuntime.cpp:1620
(anonymous namespace)::CollectRuntimeStatsRunnable::WorkerRun at dom/workers/WorkerPrivate.cpp:1480
mozilla::dom::workers::WorkerRunnable::Run at dom/workers/WorkerPrivate.cpp:1662
mozilla::dom::workers::WorkerPrivate::DoRunLoop at dom/workers/WorkerPrivate.cpp:2520
(anonymous namespace)::WorkerThreadRunnable::Run at dom/workers/RuntimeService.cpp:339
nsThread::ProcessNextEvent at xpcom/threads/nsThread.cpp:657
NS_ProcessNextEvent_P at /files/mozilla/build/a/xpcom/build/nsThreadUtils.cpp:245
nsThread::ThreadFunc at xpcom/threads/nsThread.cpp:289
_pt_root at nsprpub/pr/src/pthreads/ptthread.c:187
_pthread_start
thread_start

Is this expected? I haven't dug too much into the patch here, but it seems like the whole point of it is to make sure that stuff like this is happening on the main thread. So I'd be curious to know if this is a bug that I should be looking into, or whether I'm just misunderstanding something.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

That's probably because of the double counting bug I fixed in bug 714264.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #7)
> That's probably because of the double counting bug I fixed in bug 714264.

So it is! I just rebased and the crash went away.

You're the best, Ms2ger. I owe you some chocolate milk when we finally cross paths. ;-)