make CycleCollectedJSContext accessible from JSContext private

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

A repeated assumption in Gecko code is that a JSContext off the main thread
must be for a web worker, and so has a WorkerPrivate.  With worklets also off
the main thread (bug 1328964), these assumptions are no longer valid.

A common base class for worker and worklet avoids the need to test for a
worker or worklet thread.
I'm curious what the proposal is here.  What is going in this base class that will be shared?  Just interface or are you moving implementation details in there as well?

Would it not be possible to make worklets and workers completely separate?  Decoupling them seems like it would be nice for maintenance since they are going to be fairly different.

Also, we are trying to move more code to use virtual methods on nsIGlobalObject instead of just TLS to determine main-thread-vs-worker-thread.  I don't know if that avenue helps you here.
(In reply to Ben Kelly [:bkelly] from comment #1)
> I'm curious what the proposal is here.  What is going in this base class
> that will be shared?  Just interface or are you moving implementation
> details in there as well?

Just interface.

> Would it not be possible to make worklets and workers completely separate? 
> Decoupling them seems like it would be nice for maintenance since they are
> going to be fairly different.

The goal here is to avoid every caller having to determine whether a JSContext
belongs to a worker or worklet.  ThreadsafeIsSystemCaller() is the example
here, but others also exist.

> Also, we are trying to move more code to use virtual methods on
> nsIGlobalObject instead of just TLS to determine
> main-thread-vs-worker-thread.  I don't know if that avenue helps you here.

That could be an option, thanks.  Does it make sense to obtain an
nsIGlobalObject from a JSContext?

Please feel free to steal review, if you like.
Priority: -- → P3
Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

https://reviewboard.mozilla.org/r/224758/#review233288

I agree with bkelly. We should extend nsIGlobalObject instead of doing this.

From a JSContext* you can obtain the nsIGlobalObject doing: nsCOMPtr<nsIGlobalObject> go = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

and add a new virtual method bool IsSystemPrincipal() in nsIGlobalObject class.

::: dom/workers/RuntimeService.cpp:2830
(Diff revision 1)
>  GetWorkerPrivateFromContext(JSContext* aCx)
>  {
>    MOZ_ASSERT(!NS_IsMainThread());
>    MOZ_ASSERT(aCx);
>  
> -  void* cxPrivate = JS_GetContextPrivate(aCx);
> +  return WorkThreadPrivate::GetFor(aCx)->GetAsWorkerPrivate();

GetFor() can return null if called on a non worker and non worklet thread.

This check must be:

WorkThreadPrivate* context = WorkThreadPrivate::GetFor(aCx);
if (!context) {
  return nullptr;
}

return context->GetASWorkerPrivate();

::: dom/workers/WorkThreadPrivate.h:23
(Diff revision 1)
> +// WorkThreadPrivate is an abstract base class for WorkerPrivate and
> +// WorkletThread.
> +class WorkThreadPrivate
> +{
> +public:
> +  // Record this WorkThreadPrivate on |aCx|.  |Clear(aCx)| must be called

Move the Clear(aCx) part of the comment next to Clear().

::: dom/workers/WorkerPrivate.h:20
(Diff revision 1)
>  #include "nsTObserverArray.h"
>  
>  #include "mozilla/dom/Worker.h"
>  #include "mozilla/dom/WorkerHolder.h"
>  #include "mozilla/dom/WorkerLoadInfo.h"
> +#include "mozilla/dom/WorkThreadPrivate.h"

alphabetic order. case insensitive.
Attachment #8955676 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini [:baku] from comment #5)
> I agree with bkelly. We should extend nsIGlobalObject instead of doing this.

> and add a new virtual method bool IsSystemPrincipal() in nsIGlobalObject
> class.

Thank you for the suggestion and info on the current global.

I tried out this approach but the type of nsIGlobalObject implementation is
not feeling as good a match for choosing the method of finding the subject
principal as a JSContext private class was.

The key difference is that the type of nsIGlobalObject is not necessarily
specific to one of worker or main thread.  SimpleGlobalObject is the problem
example that I know of in current code, but I'm not aware of any clear reason
why there wouldn't be others, now or in the future.

JSContexts feel closely associated with stack/callers and subject principal,
while objects feel better suited to matters of object principal.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef38041fe4da8e1f32f5e7c677e1ee9b55c6d97&selectedJob=170196127
includes some stack traces for usage of SimpleGlobalObject::IsSystemCaller().

If an IsSystemCaller() implementation needs to test what kind of thread it
belongs to, then we don't have much value in having a virtual
nsIGlobalObject::IsSystemCaller().

Existing tests do pass when SimpleGlobalObject::IsSystemCaller() just returns
false,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a4420436a5c51dade5e9cf71c9feeb4834aa1bb
but I suspect that is not providing the desired rv for this call site
at least:

https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/xpcom/base/CycleCollectedJSRuntime.cpp#1599

Do you have any suggestions for how SimpleGlobalObject::IsSystemCaller() would
more easily determine the kind of thread than nsContentUtils?
Or shall we give up on the global object for system caller approach?
Flags: needinfo?(amarchesini)
CycleCollectedJSContext::IsSystemCaller() looks like it would be more suitable than nsIGlobalObject.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/860e07a126c0 added virtual
CycleCollectedJSContext::IsSystemCaller() and used that from
ThreadSafeIsSystemCaller(JSContext*).

However it fetches the CycleCollectedJSContext from TLS which seems a bit odd
given there is already a JSContext*.  Either we should make the
CycleCollectedJSContext accessible from the JSContext, or we should remove the
JSContext* parameter.  Removing the parameter would seem to revert some of the
work that Boris has been doing in bug 1316616 and other dependencies of bug
1316480.  I guess the intention of the JSContext parameter is to make it clear
that the JS stack will be used to determine the nature of the caller?

If we make the CycleCollectedJSContext accessible from the JSContext, then
that suggests using the context private, and so unifying the context private
among not just Worker and Worklet, but also main thread.  I'll expand this
bug for that.  I'll request review from Peter because he reviewed the
current context/runtime private code from
https://hg.mozilla.org/mozilla-central/rev/755b0e538b7a
No longer blocks: 1328964
Component: DOM: Workers → XPCOM
Summary: Use a common base class for worker and worklet data accessed from JSCont → make CycleCollectedJSContext accessible from JSContext private
Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

https://reviewboard.mozilla.org/r/224758/#review233288

> GetFor() can return null if called on a non worker and non worklet thread.
> 
> This check must be:
> 
> WorkThreadPrivate* context = WorkThreadPrivate::GetFor(aCx);
> if (!context) {
>   return nullptr;
> }
> 
> return context->GetASWorkerPrivate();

GetWorkerPrivateFromContext() currently assumes that a context private is a
WorkerThreadContextPrivate, which means that it is assuming that the thread is
a worker thread.  The WorkThreadPrivate::GetFor() proposal was making the same
assumption.

GetWorkerPrivateFromContext() does however currently provide for the
possibility that the context private has been cleared.  I'll keep that
support.
There are other places where the CycleCollectedJSContext is currently obtained
from TLS but could be obtained from a JSContext.  I haven't changed them
because they do not look like they are such that performance of a TLS lookup
will be significantly costly and calling Get() with no parameter is slightly
less code than calling GetFor(JSContext*).
Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

https://reviewboard.mozilla.org/r/224758/#review250344
Attachment #8955676 - Flags: review?(peterv) → review+
https://reviewboard.mozilla.org/r/224758/diff/2 triggers a rooting hazards
failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71cdad56f1c9f40b3a7b5205989de8ae74ba07fc&selectedJob=179769100

Function '_ZN7mozilla3dom15workerinternals12_GLOBAL__N_127WorkerThreadPrimaryRunnable3RunEv$RuntimeService.cpp:uint32 mozilla::dom::workerinternals::{anonymous}::WorkerThreadPrimaryRunnable::Run()' has unrooted 'context' of type 'class mozilla::UniquePtr<mozilla::dom::WorkerJSContext, mozilla::DefaultDelete<mozilla::dom::WorkerJSContext> >' live across GC call '_ZN7mozilla3dom15WorkerJSContext10InitializeEP9JSRuntime$uint32 mozilla::dom::WorkerJSContext::Initialize(struct JSRuntime*)' at dom/workers/RuntimeService.cpp:2688
    RuntimeService.cpp:2634: Call(1,2, asciiStr.Maybe())
    RuntimeService.cpp:2634: Call(2,3, raiiObjectLossyNsString.AutoProfilerLabel>())
    RuntimeService.cpp:2634: Call(3,4, __temp_1 := profiler_is_active())
    RuntimeService.cpp:2634: Assume(4,5, __temp_1*, true)
    RuntimeService.cpp:2634: Call(5,6, __temp_2 := this*.mWorkerPrivate*.ScriptURL())
    RuntimeService.cpp:2634: Call(6,7, asciiStr.emplace(__temp_2*))
    RuntimeService.cpp:2634: Call(7,8, __temp_4 := asciiStr.operator->())
    RuntimeService.cpp:2634: Call(8,9, __temp_3 := __temp_4*.field:0.field:0.get())
    RuntimeService.cpp:2634: Assign(9,10, __temp_5 := 2635)
    RuntimeService.cpp:2634: Assign(10,11, __temp_6 := 16)
    RuntimeService.cpp:2634: Call(11,12, raiiObjectLossyNsString.emplace("WorkerThreadPrimaryRunnable::Run",__temp_3,__temp_5,__temp_6))
    RuntimeService.cpp:2642: Call(12,13, __temp_8 := GetOrCreateForCurrentThread())
    RuntimeService.cpp:2642: Call(13,14, __temp_7 := NS_warn_if_impl(null(__temp_8*),"!BackgroundChild::GetOrCreateForCurrentThread()","/builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp",2642))
    RuntimeService.cpp:2642: Assume(14,15, __temp_7*, false)
    RuntimeService.cpp:2680: Call(15,16, __temp_9 := this*.mThread.operator 522())
    RuntimeService.cpp:2680: Call(16,17, threadHelper.SetThreadHelper(this*.mWorkerPrivate*,__temp_9*))
    RuntimeService.cpp:2682: Call(17,18, this*.mWorkerPrivate*.AssertIsOnWorkerThread())
    RuntimeService.cpp:2685: Call(18,19, nsCycleCollector_startup())
    RuntimeService.cpp:2687: Call(19,20, __temp_10 := MakeUnique(this*.mWorkerPrivate))
    RuntimeService.cpp:2688: Call(20,21, __temp_11 := context.operator->())
    RuntimeService.cpp:2688: Call(21,22, rv := __temp_11*.Initialize(this*.mParentRuntime*)) [[GC call]]
    RuntimeService.cpp:2689: Call(22,23, __temp_14 := NS_FAILED_impl(rv*))
    RuntimeService.cpp:2689: Call(23,24, __temp_13 := __builtin_expect((__temp_14* != 0),0))
    RuntimeService.cpp:2689: Call(24,25, __temp_12 := NS_warn_if_impl((__temp_13* != 0),"NS_FAILED(rv)","/builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp",2689))
    RuntimeService.cpp:2689: Assume(25,26, __temp_12*, true)
    RuntimeService.cpp:2690: Assign(26,27, return := rv*)
    RuntimeService.cpp:2690: Call(27,28, context.~WorkerJSContext> >())
GC Function: _ZN7mozilla3dom15WorkerJSContext10InitializeEP9JSRuntime$uint32 mozilla::dom::WorkerJSContext::Initialize(struct JSRuntime*)
    uint32 mozilla::CycleCollectedJSContext::Initialize(struct JSRuntime*, uint32, uint32)
    void mozilla::CycleCollectedJSContext::InitializeCommon()
    FieldCall: nsIThread.SetCanInvokeJS

Function '_ZN7mozilla3dom13WorkletThread12RunEventLoopEP9JSRuntime$void mozilla::dom::WorkletThread::RunEventLoop(struct JSRuntime*)' has unrooted 'context' of type 'class mozilla::UniquePtr<mozilla::dom::WorkletJSContext, mozilla::DefaultDelete<mozilla::dom::WorkletJSContext> >' live across GC call '_ZN7mozilla3dom16WorkletJSContext10InitializeEP9JSRuntime$uint32 mozilla::dom::WorkletJSContext::Initialize(struct JSRuntime*)' at dom/worklet/WorkletThread.cpp:340
    WorkletThread.cpp:335: Call(1,2, __temp_2 := NS_IsMainThread())
    WorkletThread.cpp:335: Call(2,3, __temp_1 := __builtin_expect(__temp_2*,0))
    WorkletThread.cpp:335: Assume(3,4, (__temp_1* != 0), true)
    WorkletThread.cpp:335: Call(4,5, MOZ_ReportAssertionFailure("!NS_IsMainThread()","/builds/worker/checkouts/gecko/dom/worklet/WorkletThread.cpp",335))
    WorkletThread.cpp:335: Assign(5,6, 0 := 335)
    WorkletThread.cpp:335: Call(6,7, abort())
    WorkletThread.cpp:337: Call(7,8, PR_SetCurrentThreadName("worklet"))
    WorkletThread.cpp:339: Assign(8,9, __temp_4 := this*)
    WorkletThread.cpp:339: Call(9,10, __temp_3 := MakeUnique(__temp_4))
    WorkletThread.cpp:340: Call(10,11, __temp_5 := context.operator->())
    WorkletThread.cpp:340: Call(11,12, rv := __temp_5*.Initialize(aParentRuntime*)) [[GC call]]
    WorkletThread.cpp:341: Call(12,13, __temp_8 := NS_FAILED_impl(rv*))
    WorkletThread.cpp:341: Call(13,14, __temp_7 := __builtin_expect((__temp_8* != 0),0))
    WorkletThread.cpp:341: Call(14,15, __temp_6 := NS_warn_if_impl((__temp_7* != 0),"NS_FAILED(rv)","/builds/worker/checkouts/gecko/dom/worklet/WorkletThread.cpp",341))
    WorkletThread.cpp:341: Assume(15,16, __temp_6*, true)
    WorkletThread.cpp:343: Call(16,32, context.~WorkletJSContext> >())
GC Function: _ZN7mozilla3dom16WorkletJSContext10InitializeEP9JSRuntime$uint32 mozilla::dom::WorkletJSContext::Initialize(struct JSRuntime*)
    uint32 mozilla::CycleCollectedJSContext::Initialize(struct JSRuntime*, uint32, uint32)
    void mozilla::CycleCollectedJSContext::InitializeCommon()
    FieldCall: nsIThread.SetCanInvokeJS

That seems to be because the rooting hazards analysis can now tell that the
jsids in PerThreadAtomCache are used across calls that may GC.
(It can now see that in WorkerJSContext and WorkletJSContext, but not in
XPCJSContext, presumably because pointers to the former remain on the stack.)

Evidence includes the fact that marking PinnedStringId as rooted suppresses
the failuire:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e66565fc3648d7a0e45ace957cd2590eca9651&selectedJob=180116249

I don't know however why a reduced testcase involving
WorkletThreadContextPrivate does not generate a similar failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4ee1bd60f832f3dd2d674e7f2d3c1a2645acd14&selectedJob=180087334
Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

https://reviewboard.mozilla.org/r/224756/#review252804

b2fd3e7b431e is just a mechanical rebase of 014be537438b.  It doesn't build.
Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

https://reviewboard.mozilla.org/r/224758/#review252806

::: xpcom/base/CycleCollectedJSContext.cpp:63
(Diff revisions 3 - 4)
>  {
>    MOZ_COUNT_CTOR(CycleCollectedJSContext);
>  
>    // Reinitialize PerThreadAtomCache because dom/bindings/Codegen.py compares
>    // against zero rather than JSID_VOID to detect uninitialized jsid members.
> -  PodZero(static_cast<PerThreadAtomCache*>(this));
> +  memset(static_cast<PerThreadAtomCache*>(this), 0, sizeof(PerThreadAtomCache));

I've reverted the change from memset to PodZero, so as not to make more work
for Jeff, who wants to deprecate PodZero.  Brace initializers are not a quick
fix here, but at least this patch will mean there is only one memset, instead
of three.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/v7EzVfPXkK4

::: dom/geolocation/nsGeolocation.cpp:10
(Diff revision 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsGeolocation.h"
>  
>  #include "mozilla/ClearOnShutdown.h"
> +#include "mozilla/CycleCollectedJSContext.h" // for nsAutoMicroTask

Two of these extra includes are now required.
Comment on attachment 8980513 [details]
bug 1442776 treat PinnedStringId as rooted GC pointer in hazard analysis

https://reviewboard.mozilla.org/r/246676/#review254590

This is ok, although I will be switching all of these to eg `class MOZ_HAZ_ROOTED PinnedStringId { ... };` or maybe `class PinnedStringId { ... } MOZ_HAZ_ROOTED;`, I can't remember which. If you #include "js/GCAnnotations.h", this *might* work now -- but then again, I think I tried doing this with all Rooted stuff already and I ran into something. So it may not work. Either way is good; I'm fine with your way of doing it for now.
Attachment #8980513 - Flags: review?(sphink) → review+
Comment on attachment 8980513 [details]
bug 1442776 treat PinnedStringId as rooted GC pointer in hazard analysis

https://reviewboard.mozilla.org/r/246676/#review254590

JS_HAZ_ROOTED works and looks better thanks, so I'll use that, thanks.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=deea638669440af51859adc15da63535ca6c22a2
Attachment #8968422 - Attachment is obsolete: true
Attachment #8968422 - Flags: review?(peterv)
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/077407e42b77
treat PinnedStringId as rooted GC pointer in hazard analysis r=sfink
https://hg.mozilla.org/integration/autoland/rev/4d39fdf74ae7
make CycleCollectedJSContext accessible from JSContext private r=peterv
Blocks: 1467023
https://hg.mozilla.org/mozilla-central/rev/077407e42b77
https://hg.mozilla.org/mozilla-central/rev/4d39fdf74ae7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.