Closed
Bug 676376
Opened 14 years ago
Closed 14 years ago
prevent multi-threaded JSRuntime access in new web worker memory reporters
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: luke, Assigned: bent.mozilla)
References
Details
Attachments
(1 file)
15.99 KB,
patch
|
luke
:
review+
sicking
:
review+
|
Details | Diff | Splinter Review |
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!
Assignee | ||
Comment 1•14 years ago
|
||
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.
Attachment #551272 -
Flags: review?(luke)
Assignee | ||
Updated•14 years ago
|
Attachment #551272 -
Flags: review?(jonas)
Comment on attachment 551272 [details] [diff] [review]
Patch, v1
Though would love a second r+ on this from luke.
Attachment #551272 -
Flags: review?(jonas) → review+
![]() |
Reporter | |
Comment 3•14 years ago
|
||
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!
Attachment #551272 -
Flags: review?(luke) → review+
Assignee | ||
Comment 4•14 years ago
|
||
Whiteboard: [inbound]
Comment 5•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Comment 6•14 years ago
|
||
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•14 years ago
|
||
That's probably because of the double counting bug I fixed in bug 714264.
Comment 8•14 years ago
|
||
(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. ;-)
You need to log in
before you can comment on or make changes to this bug.
Description
•