1248737 - Write some (more detailed) documentation for WorkerRunnable and associated things

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I'm trying to sort through it anyway; I might as well write it down as I go.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Improve documentation for WorkerRunnable and associated classes

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8719972 [details] [diff] [review]
Improve documentation for WorkerRunnable and associated classes

Review of attachment 8719972 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerRunnable.h
@@ +133,5 @@
> +  // return value will be passed to PostRun().  The JSContext passed in here
> +  // comes from an AutoJSAPI (or AutoEntryScript) that we set up on the stack.
> +  // If mBehavior is ParentThreadUnchangedBusyCount, it is in the compartment
> +  // of mWorkerPrivate's reflector (i.e. the worker object in the parent
> +  // thread), unless that reflector is inexplicably null.  For other mBehavior

It's not inexplicably null, it can be null in the case of SharedWorkers, where a single WorkerPrivate is multiplexed to SharedWorkers which are the actual DOM objects.  In that case the WorkerPrivate is not actually exposed to the DOM and thus has no reflector (or even concept of which compartment it belongs in, really)

@@ +136,5 @@
> +  // of mWorkerPrivate's reflector (i.e. the worker object in the parent
> +  // thread), unless that reflector is inexplicably null.  For other mBehavior
> +  // values, we're running on the worker thread and aCx is in whatever
> +  // compartment GetCurrentThreadJSContext() was in when nsIRunnable::Run() got
> +  // called (XXXbz: Why is this a sane thing to be doing???).  If it wasn't in

Because there is (well, was) only one.  We might have to reevaluate this now that we have a debugger too ...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> It's not inexplicably null, it can be null in the case of SharedWorkers

OK.  But as you note on IRC in that case we don't have a GetWindow() or GetParent() in that case either.  So looks like we end up in the null compartment!

But in the non-sharedworker case, when would mWorkerPrivate->GetWrapper() not be in the compartment of either mWorkerPrivate->GetWindow() or mWorkerPrivate->GetParent() (for mainthread/non-mainthread respectively)?

Seems to me like we should just assert that if !targetIsWorkerThread && GetWrapper() then its compartment matches the existing compartment on cx or something (and in particular that globalObject is non-null and is also in the same compartment).

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > It's not inexplicably null, it can be null in the case of SharedWorkers
> 
> OK.  But as you note on IRC in that case we don't have a GetWindow() or
> GetParent() in that case either.  So looks like we end up in the null
> compartment!

That may be possible, yes.  I can't imagine a sane compartment to end up in in this case other than the null compartment (i.e. we can't just pick a DOM compartment at random).

> But in the non-sharedworker case, when would mWorkerPrivate->GetWrapper()
> not be in the compartment of either mWorkerPrivate->GetWindow() or
> mWorkerPrivate->GetParent() (for mainthread/non-mainthread respectively)?

I don't think this should ever be possible *because* we always preserve the wrapper in WorkerPrivateParent<Derived>::WrapObject.  No runnables capable of running JS should be received by the parent thread after the Worker is unlinked by the CC.

> Seems to me like we should just assert that if !targetIsWorkerThread &&
> GetWrapper() then its compartment matches the existing compartment on cx or
> something (and in particular that globalObject is non-null and is also in
> the same compartment).

That sounds reasonable, yes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I tried adding those asserts, and they fail during startup of "firefox -marionette" when running tests.

When the assert fails, we have a dedicated worker.  "globalObject" is null, so "cx" is in a null compartment.  isMainThread is true.  The loadinfo's window is false, which is why global is null.  

Looking at the loadinfo some more, its base URI is resource://gre/modules/PromiseWorker.jsm and the channel URI is file://stuff/toolkit/components/osfile/modules/osfile_async_worker.js

So yeah, worker loaded on main thread from a non-window context.  That's deeply annoying.

I tried loosening up the asserts to account for this case, but I'm still failing this assert, which I thought ought to be unobjectionable:

  MOZ_ASSERT_IF(targetIsWorkerThread, globalObject);

when I fail it, JS::CurrentGlobalOrNull(cx) is null.  mWorkerPrivate->mScope and mWorkerPrivate->mDebuggerScope are both null.  The stack just shows us processing runnables via WorkerPrivate::DoRunLoop.  Our current runnable is a CompileScriptRunnable (!).

Is this a situation we should expect to end up in (possibly around worker shutdown)?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh, right.  CompileScriptRunnable is the thing that compiles the primary script and sets up the global.  Which is why we don't have a scope/global/etc.

Note that there's nothing non-window context specific about this, it just happens to be the first worker we create.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Add some asserts too

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8720434 [details] [diff] [review]
Add some asserts too

Review of attachment 8720434 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a real commit message.

::: dom/workers/WorkerRunnable.cpp
@@ +359,5 @@
> +  // _is_ the main thread, globalObject can be null for workers started from
> +  // JSMs or other non-window contexts, sadly.
> +  MOZ_ASSERT_IF(!targetIsWorkerThread && !isMainThread &&
> +                mWorkerPrivate->IsDedicatedWorker(),
> +                globalObject);

If we are a subworker we must be a dedicated worker.  Shared and Service workers only exist at the top level.  So this could actually be:

MOZ_ASSERT_IF(!targetIsWorkerThread && !isMainThread,	
              mWorkerPrivate->IsDedicatedWorker() && globalObject);

Or you could leave out the dedicated part entirely.

@@ +379,5 @@
> +  // have no globalObject) or in the compartment of our reflector.
> +  MOZ_ASSERT_IF(!targetIsWorkerThread && mWorkerPrivate->GetWrapper(),
> +                !js::GetContextCompartment(cx) ||
> +                js::GetObjectCompartment(mWorkerPrivate->GetWrapper()) ==
> +                  js::GetContextCompartment(cx));

Does it make sense to move all three of these into the if statement right before the ac.emplace?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> r=me with a real commit message.

I'm just folding it into the other patch.

> Does it make sense to move all three of these into the if statement right before the ac.emplace?

Yes, it does.  Will do.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb092fbf4fb

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cb092fbf4fb