1442776 - make CycleCollectedJSContext accessible from JSContext private

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Karl Tomlinson (:karlt)]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1442776 make CycleCollectedJSContext accessible from JSContext private

A common base class permits common calls between worker and worklet, removing
the need for callers to determine whether an off-main-thread JSContext relates
to a worker or worklet.

Safe casts to the worker- and worklet-specific data structures permit checking
whether the cast is appropriate and provide null deferences instead of wild
pointers where or if assumptions are incorrect.

Review commit: https://reviewboard.mozilla.org/r/224758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224758/

Comment 3

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dc392ed1b434a755afd3fe681269280abdfb711

Comment 4

[Karl Tomlinson (:karlt)]

(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.

Comment 5

[Andrea Marchesini [:baku]]

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.

Comment 6

[Karl Tomlinson (:karlt)]

(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?

Comment 7

[Karl Tomlinson (:karlt)]

CycleCollectedJSContext::IsSystemCaller() looks like it would be more suitable than nsIGlobalObject.

Comment 8

[Karl Tomlinson (:karlt)]

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

Comment 9

[Karl Tomlinson (:karlt)]

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.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: bug 1442776 get CycleCollectedJSContext from JSContext private when available in dom/bindings methods

Review commit: https://reviewboard.mozilla.org/r/237120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237120/

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224758/diff/1-2/

Comment 12

[Karl Tomlinson (:karlt)]

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 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

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

Comment 14

[Karl Tomlinson (:karlt)]

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 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224758/diff/2-3/

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8968422 [details]
bug 1442776 get CycleCollectedJSContext from JSContext private when available in dom/bindings methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237120/diff/1-2/

Comment 17

[Karl Tomlinson (:karlt)]

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 18

[Karl Tomlinson (:karlt)]

Attachment: bug 1442776 treat PinnedStringId as rooted GC pointer in hazard analysis

Review commit: https://reviewboard.mozilla.org/r/246676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246676/

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224758/diff/3-4/

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8968422 [details]
bug 1442776 get CycleCollectedJSContext from JSContext private when available in dom/bindings methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237120/diff/2-3/

Comment 21

[Karl Tomlinson (:karlt)]

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 22

[Steve Fink [:sfink] [:s:]]

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.

Comment 23

[Karl Tomlinson (:karlt)]

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

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8980513 [details]
bug 1442776 treat PinnedStringId as rooted GC pointer in hazard analysis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246676/diff/1-2/

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8955676 [details]
bug 1442776 make CycleCollectedJSContext accessible from JSContext private

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224758/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/077407e42b77
https://hg.mozilla.org/mozilla-central/rev/4d39fdf74ae7