757133 - Implement a Worker Debugger API

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

This is not currently a high enough priority, but here's what it'll take:

- We need a hook so that when a new Worker is started, we get to run some chrome JS code on that stack (to bootstrap). The Debugger object must be accessible there.

- We need a hook to interrupt an existing Worker and run some chrome code on its stack, again with the Debugger object accessible (to attach to a running Worker).

- The root actor needs code that uses the above features to make Workers debuggable.

- Additional remote debugging API needs to be designed for this. We need to support enumerating existing Workers, distinguishing between chrome and regular Workers, being notified when new Workers are created, inspecting their parent relationships, peeking at their message queues, being notified when messages are sent...

Comment 2

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

Andrea: This might be something you could help out with.

Comment 3

[Andrea Marchesini [:baku]]

I think this issue can be easily fixed once we have MessagePorts or any other 1:1 communication from the mainthread to a worker.
http://www.whatwg.org/specs/web-apps/current-work/#messageport

Comment 4

[Rob Campbell [:rc] (:robcee)]

Hi Andrea. Any progress on this? If you're not actively working on this bug, you should unassign yourself so we can throw it back into the pool.

Comment 5

[Andrea Marchesini [:baku]]

Rob, you are right. I was waiting for MessagePort/MessageChannel implemented in workers but I don't know when this will happen. Reassigned.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I spoke with a few of the gaia devs (who were working the mail app, iirc) and they told me that they push most everything they can into workers. If we're going to give them a good debugging experience, we really need to be able to debug workers.

Comment 8

[(no longer active)]

Are there any plans on fixing this in the near future?

Comment 9

[Rob Campbell [:rc] (:robcee)]

hopes and desires yes. Not sure of any plans.

cc'ing Eddy.

Comment 10

[Rob Campbell [:rc] (:robcee)]

and Eddy's begun investigating this...

Comment 11

[Patrick Brosset <:pbro>]

Definitely a needed feature. Also, on top of being able to debug worker scripts, I think it would be valuable to list them (maybe a separate panel/tool) and get access to some sort of statistics about them (how long have they been running, average time between a onMessage and a postMessage, when were they created, when were they ran last, ...).
This is probably a less important feature than actually being able to debug them but would still help.

Comment 12

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

For what it's worth, we're making a big push for Workers right now. Both working on adding support for a pile of APIs to them (IndexedDB, MessageChannel, Canvas, Notifications, console.*), and implementing features that rely heavily on workers (ServiceWorker).

So workers are very soon going to be a very interesting target for developers. Especially on mobile since ServiceWorker is how developers will enable offline web pages/apps.

So I think this is soon going to become a much higher priority for developers.

Comment 13

[Eddy Bruel [:ejpbruel]]

Decided to make this my Q4 project.

Comment 14

[Rob Campbell [:rc] (:robcee)]

++++

Comment 15

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

Is there an ETA here? This is going to become a blocker for having decent offline support for the web as offline will be entirely based on workers.

So we should up the priority here if it is still low?

Comment 16

[Eddy Bruel [:ejpbruel]]

(In reply to Jonas Sicking (:sicking) Vacation Nov23-Dec2. Please ni? if you want feedback from comment #15)
> Is there an ETA here? This is going to become a blocker for having decent
> offline support for the web as offline will be entirely based on workers.
> 
> So we should up the priority here if it is still low?

It's hard to give an exact ETA for this, so I'm not going to.

Currently, the goal is to be able to run debugger code in workers by the end of Q4. This requires us to be able to create auxiliary globals in each worker, as well as having a message channel to communicate with them. Getting the full debugger server up and running will likely be harder, since it uses several features not available in workers. I can give you an estimate of how long that will take when we get the first part up an running.

Comment 17

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

Can you please coordinate the implementation plan with Ben Turner who is the owner of workers. I think he is not aware that we are planning on running multiple JS globals on the worker threads.

Comment 18

[Eddy Bruel [:ejpbruel]]

Attachment: Sending and receiving debugger messages

As a first step, I've implemented an auxiliary message channel for debugger messages. This is just a prototype: I'm only putting it up here so I can get early feedback.

The final API won't live on Worker and DedicatedWorkerGlobalScope (since only the debugger script should be able to access it), but putting it there for now is convenient for prototyping.

I still need to figure out how we want to handle debug messages for SharedWorkers. Perhaps posting a message on a SharedWorkerGlobalScope should cause a ondebuggermessage event on all instances of SharedWorker (which will eventually become instances of SharedWorkerDebugger), and vice versa?

Comment 19

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

Comment on attachment 8347458 [details] [diff] [review]
Sending and receiving debugger messages

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

This looks fine, except that it's not ChromeOnly ...

I'm going to cancel this review because I don't really know what to do with it at this stage if this patch is only for prototyping.

Comment 20

[Eddy Bruel [:ejpbruel]]

Attachment: Complete patch (minus comments and tests)

This patch provides a way to get a 'debuggable' from a (Shared)Worker. A debuggable represents the debugger API for that worker. To access it, each (Shared)Worker has a 'debuggable' property, which is only available to chrome.

The debuggable itself contains functions to import debugger scripts into a worker, and to send/receive messages between these scripts. Debugger scripts are evaluated in their own separate global, and use a separate message channel from the main worker script.

The global used by debugger scripts also provides additional API functions. Most importantly, it provides a way for debugger scripts to access the global for the main worker script (so it can attach a debugger to it), and a safe way to bypass the chrome only restriction for the debuggable property on child workers (so it can load further debuggers in them).

This patch still needs extensive testing before it can land, but I'd like to get early feedback on it to see if there are any obvious problems with it.

Comment 21

[Eddy Bruel [:ejpbruel]]

Attachment: Improved patch

Improved patch, now with tests and ready for review. See comment 20 for details.

Comment 22

[Eddy Bruel [:ejpbruel]]

Attachment: Improved patch

Damn you qref!

Comment 23

[Eddy Bruel [:ejpbruel]]

I should point out that I'm currently refactoring this patch so that the main thread can talk to arbitrary workers directly (as opposed to just top level workers). This will massively simplify the server part of this task, because it allows us to have pairs of servers rather than an entire tree.

In the meantime, this task now also depends on bug 859372, which I'm also working on.

Comment 24

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

So is this something I should review or not?

If no, please cancel the review request.

Comment 25

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

This patch implements the complete API we need on the platform side to support workers in the debugger server. I've added comments and tests, and managed to get a green run on try, so it is ready to be r-d :-P

There are still a couple of places where I was unsure how to properly implement/fix something. I've marked those with TODO comments.

While this patch is pending review, I'll be shifting focus from the platform side of things to refactoring the debugger server itself, but I'll make sure to address review comments quickly, as soon as they come in.

Comment 26

[Jeff Griffiths (:canuckistani) (:⚡︎)]

cc'ing James Burke because he asks after this occasionally.

Comment 27

[Eddy Bruel [:ejpbruel]]

Friendly review ping!

Comment 28

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Who is this supposed to be review by?(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> So is this something I should review or not?

Looks like this is something you should review, or give us someone else to review it. Gaia devs tell me this is pretty important to them, I'd like to at least get it on nightly so we can get feedback from them.

Comment 29

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Griffiths (:canuckistani) from comment #28)
> Who is this supposed to be review by?(In reply to Kyle Huey [:khuey]
> ([e-mail]) from comment #24)
> > So is this something I should review or not?
> 
> Looks like this is something you should review, or give us someone else to
> review it. Gaia devs tell me this is pretty important to them, I'd like to
> at least get it on nightly so we can get feedback from them.

Just to be clear Jeff, this is just the platform part of what we need to make workers debuggable. We still need to refactor the debugger server to actually use this API, and before we can do that, we still need to get rid of all references to the Components object in the relevant code. I'm currently working on making the debugger server require friendly (bug 859372), to get rid of our dependency on Cu.import and loadSubScript.

Comment 30

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

(In reply to Jeff Griffiths (:canuckistani) from comment #28)
> Who is this supposed to be review by?(In reply to Kyle Huey [:khuey]
> ([e-mail]) from comment #24)
> > So is this something I should review or not?
> 
> Looks like this is something you should review, or give us someone else to
> review it. Gaia devs tell me this is pretty important to them, I'd like to
> at least get it on nightly so we can get feedback from them.

That question was about a previous version of the patch.

Comment 31

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Eddy Bruel [:ejpbruel] from comment #29)

> Just to be clear Jeff, this is just the platform part of what we need to
> make workers debuggable. We still need to refactor the debugger server to
> actually use this API, and before we can do that, we still need to get rid
> of all references to the Components object in the relevant code. I'm
> currently working on making the debugger server require friendly (bug
> 859372), to get rid of our dependency on Cu.import and loadSubScript.

I realize this, and also that this patch's review is blocking the subsequent work.

Comment 32

[Eddy Bruel [:ejpbruel]]

Second review ping!

Comment 33

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

Comment on attachment 8384209 [details] [diff] [review]
Patch to be reviewed

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

I skipped a few bits but I think this is a good first pass.

::: dom/webidl/WorkerDebuggerGlobalScope.webidl
@@ +11,5 @@
> +  void enterDebuggerLoop();
> +  void leaveDebuggerLoop();
> +
> +  [Throws]
> +  void postMessage(DOMString message);

Why does this not have the same signature as DedicatedWorkerGlobalScope's postMessage?

@@ +16,5 @@
> +
> +  attribute EventHandler onmessage;
> +};
> +
> +partial interface WorkerDebuggerGlobalScope {

Why is this separate?

::: dom/workers/ScriptLoader.cpp
@@ +93,5 @@
> +  switch (aScriptType) {
> +    // If this script loader is being used to make a new worker then we need
> +    // to do a same-origin check.
> +    case WorkerScript:
> +    {

{ on previous line

@@ +104,5 @@
> +      // (and other URLs that inherit their principal from their
> +      // creator.)
> +      rv = principal->CheckMayLoad(uri, false, true);
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
> +    }

break;?

@@ +109,5 @@
> +
> +    // TODO: We want to be able to load debugger scripts from any location,
> +    // regardless of whether the worker is a chrome or content worker. Do we
> +    // need to do any kind of check for this?
> +    case DebuggerScript:

Why do we want to load debugger scripts from anything other than a chrome URI?

@@ +358,5 @@
> +
> +    case DebuggerScript:
> +      // TODO: What could we use as a base URI for debugger scripts?
> +      baseURI = nullptr;
> +      break;

I don't think there's a useful concept here, especially if we're only loading chrome URIs.  This is really a bz question though.

::: dom/workers/WorkerDebuggerManager.cpp
@@ +16,5 @@
> +
> +class WorkerDebuggerEnumerator MOZ_FINAL : public nsISimpleEnumerator
> +{
> +  uint32_t mIndex;
> +  nsCOMArray<nsISupports> mArray;

Please use nsTArray<nsCOMPtr<nsISupports>>.  nsCOMArray is old and deprecated.

@@ +55,5 @@
> +  if (mIndex == mArray.Length()) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  nsCOMPtr<nsISupports> element = mArray.ElementAt(mIndex++);
> +  *aResult = element.forget().get();

element.forget(aResult);

@@ +59,5 @@
> +  *aResult = element.forget().get();
> +  return NS_OK;
> +};
> +
> +StaticRefPtr<WorkerDebuggerManager> gWorkerDebuggerManager;

You don't need a refptr here.  The service manager will hold your object alive until shutdown.

@@ +83,5 @@
> +{
> +  // TODO: Even after setting this to null, we still have outstanding references
> +  // to the worker debugger manager. Where do these references come from, and
> +  // how can we make sure our destructor gets called at this point?
> +  gWorkerDebuggerManager = nullptr;

From the XPCOM service manager?  Or from JS?

@@ +107,5 @@
> +  MutexAutoLock lock(mMutex);
> +
> +  nsRefPtr<WorkerDebuggerEnumerator> enumerator =
> +    new WorkerDebuggerEnumerator(mDebuggers);
> +  *aResult = enumerator.forget().get();

enumerator.forget(aResult);

@@ +118,5 @@
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mDebuggerRegisteredCallback;
> +  *aResult = callback.forget().get();

callback.forget(aResult);

@@ +139,5 @@
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mDebuggerUnregisteredCallback;
> +  *aResult = callback.forget().get();

...

@@ +154,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +WorkerDebuggerManager::RegisterDebugger(nsIWorkerDebugger* aDebugger)

This should probably take the concrete type, not the interface.  Also, you should assert that we are on the debugger's thread (by calling a method on the debugger, presumably).

@@ +163,5 @@
> +  mDebuggers->PutEntry(aDebugger);
> +
> +  if (mDebuggerRegisteredCallback) {
> +    nsCOMPtr<nsIRunnable> runnable =
> +       NS_NewRunnableMethodWithArg<nsCOMPtr<nsIWorkerDebugger> >(this,

>> is supported on all our compilers now.  Use it.

@@ +172,5 @@
> +    }
> +
> +    /* Wait for the callback to return. This gives us a chance to import
> +     * debugger scripts before any worker code starts running.
> +     */

I would make this comment more specific ... something about how blocking regular execution until the debugger is ready lets it front-run the worker code.

@@ +173,5 @@
> +
> +    /* Wait for the callback to return. This gives us a chance to import
> +     * debugger scripts before any worker code starts running.
> +     */
> +    mCondVar.Wait();

This doesn't work.  What happens if two threads are blocking on this condvar registering debuggers at the same time?  The first TriggerDebuggerRegisteredCallback will wake up a random thread.  I think you need a separate condvar for each WorkerDebugger.  That or we need to wake all the threads and the ones that are not ready to proceed need to go back to sleep.

@@ +179,5 @@
> +}
> +
> +void
> +WorkerDebuggerManager::UnregisterDebugger(nsIWorkerDebugger* aDebugger)
> +{

Same things here.

::: dom/workers/WorkerDebuggerManager.h
@@ +29,5 @@
> +  mozilla::Mutex mMutex;
> +  mozilla::CondVar mCondVar;
> +  nsCOMPtr<nsIWorkerDebuggerCallback> mDebuggerRegisteredCallback;
> +  nsCOMPtr<nsIWorkerDebuggerCallback> mDebuggerUnregisteredCallback;
> +  WorkerDebuggerTable* mDebuggers;

Why is this a pointer?

::: dom/workers/WorkerPrivate.cpp
@@ +1145,5 @@
>      return aWorkerPrivate->ResumeInternal(aCx);
>    }
>  };
>  
> +class LogErrorRunnable MOZ_FINAL : public nsIRunnable

This needs a better name ... something with the console in it.  Also it should inherit from nsRunnable.

@@ +1870,5 @@
> +    JS::Rooted<JSObject*> global(aCx, globalScope->GetWrapper());
> +
> +    JSAutoCompartment ac(aCx, global);
> +
> +    return scriptloader::LoadDebuggerScript(aCx, mScriptURL);

nit: no newline between ac and the return.

@@ +2358,5 @@
> +WorkerPrivate::WorkerDebugger::GetWindow(nsIDOMWindow** aResult)
> +{
> +  AssertIsOnMainThread();
> +
> +  nsRefPtr<nsPIDOMWindow> window = nullptr;

nsRef/COMPtr don't need to be initialized to null.

@@ +2360,5 @@
> +  AssertIsOnMainThread();
> +
> +  nsRefPtr<nsPIDOMWindow> window = nullptr;
> +  {
> +    MutexAutoLock lock(mMutex);

No need for the extra bracing.  Just hold this for the entire function.

@@ +2363,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +
> +    if (!mWorkerPrivate)
> +      return NS_ERROR_UNEXPECTED;

brace single line control statements.  We don't want any goto fails here.

@@ +2370,5 @@
> +    // parent cannot be destroyed, etc. This allows us to walk up the parent
> +    // chain to obtain the window from the top-level worker.
> +    WorkerPrivate* workerPrivate = mWorkerPrivate;
> +    while (workerPrivate->GetParent())
> +      workerPrivate = workerPrivate->GetParent();

here too

@@ +2374,5 @@
> +      workerPrivate = workerPrivate->GetParent();
> +    window = workerPrivate->GetWindow();
> +  }
> +
> +  *aResult = window.forget().get();

window.forget(aResult);

@@ +2385,5 @@
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  nsCOMPtr<nsIWorkerDebuggerMessageCallback> callback = mMessageCallback;
> +  *aResult = callback.forget().get();

here too

@@ +2406,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  {
> +    MutexAutoLock lock(mMutex);

here too

@@ +2429,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  {
> +    MutexAutoLock lock(mMutex);

and here

@@ +2452,5 @@
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mClosedCallback;
> +  *aResult = callback.forget().get();

...

@@ +2591,5 @@
>        return rv;
>      }
>  
> +    // Postpone notifying the condition variable if we are inside a debugger
> +    // loop so that we do not tightly spin the loop.

This comment doesn't make sense to me.  I assume what you're trying to say is that if we're in the debugger loop we don't want to wake up the thread for normal runnables?

@@ +2593,5 @@
>  
> +    // Postpone notifying the condition variable if we are inside a debugger
> +    // loop so that we do not tightly spin the loop.
> +    if (self->mDebuggerLoopStackDepth == 0)
> +      mCondVar.Notify();

braces

@@ +2642,5 @@
>  
>  template <class Derived>
> +nsresult
> +WorkerPrivateParent<Derived>::DispatchDebuggerRunnable(
> +                                WorkerDebuggerRunnable* aWorkerDebuggerRunnable)

Should we be asserting here that we're in a debugger loop or something?

@@ +2653,5 @@
> +
> +  WorkerPrivate* self = ParentAsWorkerPrivate();
> +
> +  {
> +    MutexAutoLock lock(mMutex);

here too

@@ +2659,5 @@
> +    // If the worker is dead, the worker debugger must have been unregistered.
> +    MOZ_ASSERT(self->mStatus != Dead);
> +
> +    // Transfer ownership to the debugger queue.
> +    self->mDebuggerQueue.Push(runnable.forget().get());

This is .forget().take() now.

@@ +4362,5 @@
> +{
> +  AssertIsOnWorkerThread();
> +
> +  {
> +    MutexAutoLock lock(mMutex);

here too

@@ +4369,5 @@
> +  }
> +}
> +
> +void
> +WorkerPrivate::LeaveDebuggerLoop()

This should assert that we don't go negative.

@@ +4374,5 @@
> +{
> +  AssertIsOnWorkerThread();
> +
> +  {
> +    MutexAutoLock lock(mMutex);

here too.

@@ +4380,5 @@
> +    // If this is the last debugger loop then we should signal the worker to
> +    // process any normal runnables that were dispatched while the debugger
> +    // loop was spinning.
> +    if (--mDebuggerLoopStackDepth == 0)
> +      mCondVar.Notify();

This doesn't make any sense.  The only thread that ever waits on the condvar is the thread that we're currently on ...

@@ +4408,5 @@
>    for (;;) {
>      // Workers lazily create a global object in CompileScriptRunnable. We need
>      // to enter the global's compartment as soon as it has been created.
>      if (workerCompartment.empty()) {
> +      if (GlobalScope()) {

alias this locally so you can reuse it below

@@ -4030,5 @@
> -    // Nothing to do here if we don't have any runnables in the main queue.
> -    if (!normalRunnablesPending) {
> -      SetGCTimerMode(IdleTimer);
> -      continue;
> -    }

I would prefer that you kept the if (!pending) continue logic in this order.  It's a nice shortcut that leads to much less nesting.

@@ +4525,5 @@
> +        }
> +
> +        // Process a single runnable from the debugger queue.
> +        static_cast<nsIRunnable*>(runnable)->Run();
> +        runnable->Release();

I sort of think that everything above this up to the if (debuggerRunnablesPending) should go in a ProcessDebuggerEvent function.

@@ +4533,5 @@
> +          // decision.
> +          if (!debuggerCompartment.empty()) {
> +            JS_MaybeGC(aCx);
> +          }
> +        }

This GC logic should be coalesced.

::: dom/workers/WorkerRunnable.h
@@ +344,5 @@
>  
> +// This runnable is used to communicate from the worker debugger on the main
> +// thread to the worker. It runs before any previously queued runnables (with
> +// the exception of control runnables), but does not interrupt any JS code
> +// executing on the stack.

Mention how this interacts with sync loops.

@@ +345,5 @@
> +// This runnable is used to communicate from the worker debugger on the main
> +// thread to the worker. It runs before any previously queued runnables (with
> +// the exception of control runnables), but does not interrupt any JS code
> +// executing on the stack.
> +class WorkerDebuggerRunnable : public WorkerRunnable

Why doesn't this inherit from MainThreadWorkerRunnable?

::: dom/workers/WorkerScope.cpp
@@ +375,5 @@
> +WorkerDebuggerGlobalScope::AddWorkerGlobalAsDebuggee(
> +                                               JSContext* aCx,
> +                                               JS::Handle<JSObject *> aDebugger,
> +                                               ErrorResult& aRv)
> +{

Add a threading assertion.

@@ +419,5 @@
> +{
> +  return mWorkerPrivate->PostDebuggerMessageToMainThread(aMessage, aRv);
> +}
> +
> +static const JSClass workerdebuggersandbox_class = {

Somebody else (e.g. bholley) should review this.

@@ +446,5 @@
> +
> +  {
> +    JSAutoCompartment ac(aCx, obj);
> +
> +    if (!JS_InitStandardClasses(aCx, obj)) {

JS_InitStandardClasses (certainly used to) fails intermittently off the main thread.  See bug 934889.

::: dom/workers/nsIWorkerDebugger.idl
@@ +11,5 @@
> +[scriptable, function, uuid(54fd2dd3-c01b-4f71-888f-462f37a54f57)]
> +interface nsIWorkerDebuggerCallback : nsISupports
> +{
> +  void callback(in nsIWorkerDebugger debugger);
> +};

You should give these better names than 'callback' in case someone wants to implement them on an object and not as a function.

Comment 34

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #33)
> Comment on attachment 8384209 [details] [diff] [review]
> Patch to be reviewed
> 
> Review of attachment 8384209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I skipped a few bits but I think this is a good first pass.
> 

First off, thanks for reviewing this! It's a rather big patch, so I appreciate your time.

Before I start working on the second version, I have a couple of comments/questions:

> ::: dom/webidl/WorkerDebuggerGlobalScope.webidl
> @@ +11,5 @@
> > +  void enterDebuggerLoop();
> > +  void leaveDebuggerLoop();
> > +
> > +  [Throws]
> > +  void postMessage(DOMString message);
> 
> Why does this not have the same signature as DedicatedWorkerGlobalScope's
> postMessage?

Mainly for symmetry reasons.

I initially tried to use the same signatures as the postMessage method and onmessage event handler on Worker. However, since nsIWorkerDebugger is an XPCOM interface, I can't easily use variadic arguments there. Moreover, because the debugger server only relies on pure JSON messages, we don't care about structured cloning for the debugger API.

The easiest thing to do seemed to be to use a simplified form of the API on both sides, which doesn't support structured cloning, and has the same signature on both ends.

> 
> @@ +16,5 @@
> > +
> > +  attribute EventHandler onmessage;
> > +};
> > +
> > +partial interface WorkerDebuggerGlobalScope {
> 
> Why is this separate?
> 

No real reason. Are there reasons not to keep this separate?

> ::: dom/workers/ScriptLoader.cpp
> @@ +93,5 @@
> > +  switch (aScriptType) {
> > +    // If this script loader is being used to make a new worker then we need
> > +    // to do a same-origin check.
> > +    case WorkerScript:
> > +    {
> 
> { on previous line
> 
> @@ +104,5 @@
> > +      // (and other URLs that inherit their principal from their
> > +      // creator.)
> > +      rv = principal->CheckMayLoad(uri, false, true);
> > +      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
> > +    }
> 
> break;?
> 

Ok.

> @@ +109,5 @@
> > +
> > +    // TODO: We want to be able to load debugger scripts from any location,
> > +    // regardless of whether the worker is a chrome or content worker. Do we
> > +    // need to do any kind of check for this?
> > +    case DebuggerScript:
> 
> Why do we want to load debugger scripts from anything other than a chrome
> URI?
> 

We don't. I had assumed that being able to load chrome URLs implied that you can load *any* URL. Is this not the case?

> @@ +358,5 @@
> > +
> > +    case DebuggerScript:
> > +      // TODO: What could we use as a base URI for debugger scripts?
> > +      baseURI = nullptr;
> > +      break;
> 
> I don't think there's a useful concept here, especially if we're only
> loading chrome URIs.  This is really a bz question though.
> 
> ::: dom/workers/WorkerDebuggerManager.cpp
> @@ +16,5 @@
> > +
> > +class WorkerDebuggerEnumerator MOZ_FINAL : public nsISimpleEnumerator
> > +{
> > +  uint32_t mIndex;
> > +  nsCOMArray<nsISupports> mArray;
> 
> Please use nsTArray<nsCOMPtr<nsISupports>>.  nsCOMArray is old and
> deprecated.
> 

Ok. I'll ask bz about the baseURI stuff.

> @@ +55,5 @@
> > +  if (mIndex == mArray.Length()) {
> > +    return NS_ERROR_UNEXPECTED;
> > +  }
> > +  nsCOMPtr<nsISupports> element = mArray.ElementAt(mIndex++);
> > +  *aResult = element.forget().get();
> 
> element.forget(aResult);

Ah, that looks a lot better. Thanks for pointing that out.

> 
> @@ +59,5 @@
> > +  *aResult = element.forget().get();
> > +  return NS_OK;
> > +};
> > +
> > +StaticRefPtr<WorkerDebuggerManager> gWorkerDebuggerManager;
> 
> You don't need a refptr here.  The service manager will hold your object
> alive until shutdown.
> 
> @@ +83,5 @@
> > +{
> > +  // TODO: Even after setting this to null, we still have outstanding references
> > +  // to the worker debugger manager. Where do these references come from, and
> > +  // how can we make sure our destructor gets called at this point?
> > +  gWorkerDebuggerManager = nullptr;
> 
> From the XPCOM service manager?  Or from JS?
> 

I don't understand your question.

> @@ +107,5 @@
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  nsRefPtr<WorkerDebuggerEnumerator> enumerator =
> > +    new WorkerDebuggerEnumerator(mDebuggers);
> > +  *aResult = enumerator.forget().get();
> 
> enumerator.forget(aResult);
> 
> @@ +118,5 @@
> > +{
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mDebuggerRegisteredCallback;
> > +  *aResult = callback.forget().get();
> 
> callback.forget(aResult);
> 
> @@ +139,5 @@
> > +{
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mDebuggerUnregisteredCallback;
> > +  *aResult = callback.forget().get();
> 
> ...
> 
> @@ +154,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +void
> > +WorkerDebuggerManager::RegisterDebugger(nsIWorkerDebugger* aDebugger)
> 
> This should probably take the concrete type, not the interface.  Also, you
> should assert that we are on the debugger's thread (by calling a method on
> the debugger, presumably).
> 

I just copied what MemoryReporter does here. Do you feel strongly about changing this?

> @@ +163,5 @@
> > +  mDebuggers->PutEntry(aDebugger);
> > +
> > +  if (mDebuggerRegisteredCallback) {
> > +    nsCOMPtr<nsIRunnable> runnable =
> > +       NS_NewRunnableMethodWithArg<nsCOMPtr<nsIWorkerDebugger> >(this,
> 
> >> is supported on all our compilers now.  Use it.
> 

Really? That's news to me. Awesome.

> @@ +172,5 @@
> > +    }
> > +
> > +    /* Wait for the callback to return. This gives us a chance to import
> > +     * debugger scripts before any worker code starts running.
> > +     */
> 
> I would make this comment more specific ... something about how blocking
> regular execution until the debugger is ready lets it front-run the worker
> code.
> 
> @@ +173,5 @@
> > +
> > +    /* Wait for the callback to return. This gives us a chance to import
> > +     * debugger scripts before any worker code starts running.
> > +     */
> > +    mCondVar.Wait();
> 
> This doesn't work.  What happens if two threads are blocking on this condvar
> registering debuggers at the same time?  The first
> TriggerDebuggerRegisteredCallback will wake up a random thread.  I think you
> need a separate condvar for each WorkerDebugger.  That or we need to wake
> all the threads and the ones that are not ready to proceed need to go back
> to sleep.

Whoa, good catch!

> 
> @@ +179,5 @@
> > +}
> > +
> > +void
> > +WorkerDebuggerManager::UnregisterDebugger(nsIWorkerDebugger* aDebugger)
> > +{
> 
> Same things here.
> 
> ::: dom/workers/WorkerDebuggerManager.h
> @@ +29,5 @@
> > +  mozilla::Mutex mMutex;
> > +  mozilla::CondVar mCondVar;
> > +  nsCOMPtr<nsIWorkerDebuggerCallback> mDebuggerRegisteredCallback;
> > +  nsCOMPtr<nsIWorkerDebuggerCallback> mDebuggerUnregisteredCallback;
> > +  WorkerDebuggerTable* mDebuggers;
> 
> Why is this a pointer?

I just copied what MemoryReporter did here. Now that I look at it it probably makes no sense to make this a pointer.

> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +1145,5 @@
> >      return aWorkerPrivate->ResumeInternal(aCx);
> >    }
> >  };
> >  
> > +class LogErrorRunnable MOZ_FINAL : public nsIRunnable
> 
> This needs a better name ... something with the console in it.  Also it
> should inherit from nsRunnable.

I actually refactored this part of the patch yesterday so we can have actual error events on the WorkerDebugger, which should make testing a whole lot easier!

> 
> @@ +1870,5 @@
> > +    JS::Rooted<JSObject*> global(aCx, globalScope->GetWrapper());
> > +
> > +    JSAutoCompartment ac(aCx, global);
> > +
> > +    return scriptloader::LoadDebuggerScript(aCx, mScriptURL);
> 
> nit: no newline between ac and the return.
> 
> @@ +2358,5 @@
> > +WorkerPrivate::WorkerDebugger::GetWindow(nsIDOMWindow** aResult)
> > +{
> > +  AssertIsOnMainThread();
> > +
> > +  nsRefPtr<nsPIDOMWindow> window = nullptr;
> 
> nsRef/COMPtr don't need to be initialized to null.
> 
> @@ +2360,5 @@
> > +  AssertIsOnMainThread();
> > +
> > +  nsRefPtr<nsPIDOMWindow> window = nullptr;
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> No need for the extra bracing.  Just hold this for the entire function.
> 
> @@ +2363,5 @@
> > +  {
> > +    MutexAutoLock lock(mMutex);
> > +
> > +    if (!mWorkerPrivate)
> > +      return NS_ERROR_UNEXPECTED;
> 
> brace single line control statements.  We don't want any goto fails here.
> 
> @@ +2370,5 @@
> > +    // parent cannot be destroyed, etc. This allows us to walk up the parent
> > +    // chain to obtain the window from the top-level worker.
> > +    WorkerPrivate* workerPrivate = mWorkerPrivate;
> > +    while (workerPrivate->GetParent())
> > +      workerPrivate = workerPrivate->GetParent();
> 
> here too
> 
> @@ +2374,5 @@
> > +      workerPrivate = workerPrivate->GetParent();
> > +    window = workerPrivate->GetWindow();
> > +  }
> > +
> > +  *aResult = window.forget().get();
> 
> window.forget(aResult);
> 
> @@ +2385,5 @@
> > +{
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  nsCOMPtr<nsIWorkerDebuggerMessageCallback> callback = mMessageCallback;
> > +  *aResult = callback.forget().get();
> 
> here too
> 
> @@ +2406,5 @@
> > +{
> > +  AssertIsOnMainThread();
> > +
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> here too
> 
> @@ +2429,5 @@
> > +{
> > +  AssertIsOnMainThread();
> > +
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> and here
> 
> @@ +2452,5 @@
> > +{
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  nsCOMPtr<nsIWorkerDebuggerCallback> callback = mClosedCallback;
> > +  *aResult = callback.forget().get();
> 
> ...
> 
> @@ +2591,5 @@
> >        return rv;
> >      }
> >  
> > +    // Postpone notifying the condition variable if we are inside a debugger
> > +    // loop so that we do not tightly spin the loop.
> 
> This comment doesn't make sense to me.  I assume what you're trying to say
> is that if we're in the debugger loop we don't want to wake up the thread
> for normal runnables?

That's correct. I'll try to clarify this comment.

> 
> @@ +2593,5 @@
> >  
> > +    // Postpone notifying the condition variable if we are inside a debugger
> > +    // loop so that we do not tightly spin the loop.
> > +    if (self->mDebuggerLoopStackDepth == 0)
> > +      mCondVar.Notify();
> 
> braces
> 
> @@ +2642,5 @@
> >  
> >  template <class Derived>
> > +nsresult
> > +WorkerPrivateParent<Derived>::DispatchDebuggerRunnable(
> > +                                WorkerDebuggerRunnable* aWorkerDebuggerRunnable)
> 
> Should we be asserting here that we're in a debugger loop or something?

No. debugger runnables can also be dispatched if we're not in a debugger loop, but in that case, they don't take precedence over normal runnables. Debugger loops ensure that the worker won't process normal runnables, but we can still talk to the debugger when the worker is paused.

> 
> @@ +2653,5 @@
> > +
> > +  WorkerPrivate* self = ParentAsWorkerPrivate();
> > +
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> here too
> 
> @@ +2659,5 @@
> > +    // If the worker is dead, the worker debugger must have been unregistered.
> > +    MOZ_ASSERT(self->mStatus != Dead);
> > +
> > +    // Transfer ownership to the debugger queue.
> > +    self->mDebuggerQueue.Push(runnable.forget().get());
> 
> This is .forget().take() now.
> 
> @@ +4362,5 @@
> > +{
> > +  AssertIsOnWorkerThread();
> > +
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> here too
> 
> @@ +4369,5 @@
> > +  }
> > +}
> > +
> > +void
> > +WorkerPrivate::LeaveDebuggerLoop()
> 
> This should assert that we don't go negative.
> 
> @@ +4374,5 @@
> > +{
> > +  AssertIsOnWorkerThread();
> > +
> > +  {
> > +    MutexAutoLock lock(mMutex);
> 
> here too.
> 
> @@ +4380,5 @@
> > +    // If this is the last debugger loop then we should signal the worker to
> > +    // process any normal runnables that were dispatched while the debugger
> > +    // loop was spinning.
> > +    if (--mDebuggerLoopStackDepth == 0)
> > +      mCondVar.Notify();
> 
> This doesn't make any sense.  The only thread that ever waits on the condvar
> is the thread that we're currently on ...

Yeah, you're right, now that I think about it. What should happen is that after this method is called we drop back into WorkerPrivate::DoRunLoop, which checks if there are any normal runnables before it decides to block.

> 
> @@ +4408,5 @@
> >    for (;;) {
> >      // Workers lazily create a global object in CompileScriptRunnable. We need
> >      // to enter the global's compartment as soon as it has been created.
> >      if (workerCompartment.empty()) {
> > +      if (GlobalScope()) {
> 
> alias this locally so you can reuse it below
> 
> @@ -4030,5 @@
> > -    // Nothing to do here if we don't have any runnables in the main queue.
> > -    if (!normalRunnablesPending) {
> > -      SetGCTimerMode(IdleTimer);
> > -      continue;
> > -    }
> 
> I would prefer that you kept the if (!pending) continue logic in this order.
> It's a nice shortcut that leads to much less nesting.
> 
> @@ +4525,5 @@
> > +        }
> > +
> > +        // Process a single runnable from the debugger queue.
> > +        static_cast<nsIRunnable*>(runnable)->Run();
> > +        runnable->Release();
> 
> I sort of think that everything above this up to the if
> (debuggerRunnablesPending) should go in a ProcessDebuggerEvent function.

That's fine by me.

> 
> @@ +4533,5 @@
> > +          // decision.
> > +          if (!debuggerCompartment.empty()) {
> > +            JS_MaybeGC(aCx);
> > +          }
> > +        }
> 
> This GC logic should be coalesced.

With the JS_MaybeGC(aCx) further up? Note that we're in different compartments in each case. 

> 
> ::: dom/workers/WorkerRunnable.h
> @@ +344,5 @@
> >  
> > +// This runnable is used to communicate from the worker debugger on the main
> > +// thread to the worker. It runs before any previously queued runnables (with
> > +// the exception of control runnables), but does not interrupt any JS code
> > +// executing on the stack.
> 
> Mention how this interacts with sync loops.
> 
> @@ +345,5 @@
> > +// This runnable is used to communicate from the worker debugger on the main
> > +// thread to the worker. It runs before any previously queued runnables (with
> > +// the exception of control runnables), but does not interrupt any JS code
> > +// executing on the stack.
> > +class WorkerDebuggerRunnable : public WorkerRunnable
> 
> Why doesn't this inherit from MainThreadWorkerRunnable?
> 

I don't think that exists. There's MainThreadWorkerSyncRunnable and MainThreadWorkerControlRunnable, neither of which are appropriate here.

> ::: dom/workers/WorkerScope.cpp
> @@ +375,5 @@
> > +WorkerDebuggerGlobalScope::AddWorkerGlobalAsDebuggee(
> > +                                               JSContext* aCx,
> > +                                               JS::Handle<JSObject *> aDebugger,
> > +                                               ErrorResult& aRv)
> > +{
> 
> Add a threading assertion.
> 
> @@ +419,5 @@
> > +{
> > +  return mWorkerPrivate->PostDebuggerMessageToMainThread(aMessage, aRv);
> > +}
> > +
> > +static const JSClass workerdebuggersandbox_class = {
> 
> Somebody else (e.g. bholley) should review this.

This needs some rethinking anyway, as it turns out we almost never want to eval code in a sandbox, we want to be able to load a URL in a sandbox. How do you feel about extending importScript so it can take an optional global (we would only expose this functionality to the worker debugger ofc).

> 
> @@ +446,5 @@
> > +
> > +  {
> > +    JSAutoCompartment ac(aCx, obj);
> > +
> > +    if (!JS_InitStandardClasses(aCx, obj)) {
> 
> JS_InitStandardClasses (certainly used to) fails intermittently off the main
> thread.  See bug 934889.

I'll look into it. 

> 
> ::: dom/workers/nsIWorkerDebugger.idl
> @@ +11,5 @@
> > +[scriptable, function, uuid(54fd2dd3-c01b-4f71-888f-462f37a54f57)]
> > +interface nsIWorkerDebuggerCallback : nsISupports
> > +{
> > +  void callback(in nsIWorkerDebugger debugger);
> > +};
> 
> You should give these better names than 'callback' in case someone wants to
> implement them on an object and not as a function.

Any suggestions? I looked around in existing idl files for inspiration, but couldn't come up with anything good.

Comment 35

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

(In reply to Eddy Bruel [:ejpbruel] from comment #34)
> > @@ +16,5 @@
> > > +
> > > +  attribute EventHandler onmessage;
> > > +};
> > > +
> > > +partial interface WorkerDebuggerGlobalScope {
> > 
> > Why is this separate?
> > 
> 
> No real reason. Are there reasons not to keep this separate?

If there's no reason to split the interface then lets not split the interface.

> > @@ +109,5 @@
> > > +
> > > +    // TODO: We want to be able to load debugger scripts from any location,
> > > +    // regardless of whether the worker is a chrome or content worker. Do we
> > > +    // need to do any kind of check for this?
> > > +    case DebuggerScript:
> > 
> > Why do we want to load debugger scripts from anything other than a chrome
> > URI?
> > 
> 
> We don't. I had assumed that being able to load chrome URLs implied that you
> can load *any* URL. Is this not the case?

I don't know off hand.  Test it?

> > 
> > @@ +59,5 @@
> > > +  *aResult = element.forget().get();
> > > +  return NS_OK;
> > > +};
> > > +
> > > +StaticRefPtr<WorkerDebuggerManager> gWorkerDebuggerManager;
> > 
> > You don't need a refptr here.  The service manager will hold your object
> > alive until shutdown.
> > 
> > @@ +83,5 @@
> > > +{
> > > +  // TODO: Even after setting this to null, we still have outstanding references
> > > +  // to the worker debugger manager. Where do these references come from, and
> > > +  // how can we make sure our destructor gets called at this point?
> > > +  gWorkerDebuggerManager = nullptr;
> > 
> > From the XPCOM service manager?  Or from JS?
> > 
> 
> I don't understand your question.

Those are suggestions of where you might look for your reference.

> > @@ +154,5 @@
> > > +  return NS_OK;
> > > +}
> > > +
> > > +void
> > > +WorkerDebuggerManager::RegisterDebugger(nsIWorkerDebugger* aDebugger)
> > 
> > This should probably take the concrete type, not the interface.  Also, you
> > should assert that we are on the debugger's thread (by calling a method on
> > the debugger, presumably).
> > 
> 
> I just copied what MemoryReporter does here. Do you feel strongly about
> changing this?

No.

> > @@ +173,5 @@
> > > +
> > > +    /* Wait for the callback to return. This gives us a chance to import
> > > +     * debugger scripts before any worker code starts running.
> > > +     */
> > > +    mCondVar.Wait();
> > 
> > This doesn't work.  What happens if two threads are blocking on this condvar
> > registering debuggers at the same time?  The first
> > TriggerDebuggerRegisteredCallback will wake up a random thread.  I think you
> > need a separate condvar for each WorkerDebugger.  That or we need to wake
> > all the threads and the ones that are not ready to proceed need to go back
> > to sleep.
> 
> Whoa, good catch!

Write a test ;-)

> > @@ +2642,5 @@
> > >  
> > >  template <class Derived>
> > > +nsresult
> > > +WorkerPrivateParent<Derived>::DispatchDebuggerRunnable(
> > > +                                WorkerDebuggerRunnable* aWorkerDebuggerRunnable)
> > 
> > Should we be asserting here that we're in a debugger loop or something?
> 
> No. debugger runnables can also be dispatched if we're not in a debugger
> loop, but in that case, they don't take precedence over normal runnables.
> Debugger loops ensure that the worker won't process normal runnables, but we
> can still talk to the debugger when the worker is paused.
> 

Is that true?  I thought we were executing debugger runnables preferentially regardless.  It's just that if we're in a debugger loop we don't process normal runnables at all.

> > @@ +4380,5 @@
> > > +    // If this is the last debugger loop then we should signal the worker to
> > > +    // process any normal runnables that were dispatched while the debugger
> > > +    // loop was spinning.
> > > +    if (--mDebuggerLoopStackDepth == 0)
> > > +      mCondVar.Notify();
> > 
> > This doesn't make any sense.  The only thread that ever waits on the condvar
> > is the thread that we're currently on ...
> 
> Yeah, you're right, now that I think about it. What should happen is that
> after this method is called we drop back into WorkerPrivate::DoRunLoop,
> which checks if there are any normal runnables before it decides to block.

Yeah that sounds right.

> > 
> > @@ +4533,5 @@
> > > +          // decision.
> > > +          if (!debuggerCompartment.empty()) {
> > > +            JS_MaybeGC(aCx);
> > > +          }
> > > +        }
> > 
> > This GC logic should be coalesced.
> 
> With the JS_MaybeGC(aCx) further up? Note that we're in different
> compartments in each case. 

Mmm, true.

> > @@ +345,5 @@
> > > +// This runnable is used to communicate from the worker debugger on the main
> > > +// thread to the worker. It runs before any previously queued runnables (with
> > > +// the exception of control runnables), but does not interrupt any JS code
> > > +// executing on the stack.
> > > +class WorkerDebuggerRunnable : public WorkerRunnable
> > 
> > Why doesn't this inherit from MainThreadWorkerRunnable?
> > 
> 
> I don't think that exists. There's MainThreadWorkerSyncRunnable and
> MainThreadWorkerControlRunnable, neither of which are appropriate here.

Maybe that's being added in bug 949325 and just hasn't landed yet then.

> > 
> > ::: dom/workers/nsIWorkerDebugger.idl
> > @@ +11,5 @@
> > > +[scriptable, function, uuid(54fd2dd3-c01b-4f71-888f-462f37a54f57)]
> > > +interface nsIWorkerDebuggerCallback : nsISupports
> > > +{
> > > +  void callback(in nsIWorkerDebugger debugger);
> > > +};
> > 
> > You should give these better names than 'callback' in case someone wants to
> > implement them on an object and not as a function.
> 
> Any suggestions? I looked around in existing idl files for inspiration, but
> couldn't come up with anything good.

onDebuggerChange/onMessage, perhaps?

Comment 36

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v2)

I think this patch is ready for another round of review.

Changes wrt the previous patch:
- Fixed the race condition in worker debugger registration
- Improved error handling to have error events for worker debuggers
- Refactor the sandbox API to better match what we actually need
- Addressed miscellaneous review comments

Things that still need to happen:
- I would like to add a test for the race condition in worker debugger registration, but I'm not sure how to go about it. Any suggestions?
- Make sure that JS_InitStandardClasses doesn't fail intermittently off main-thread

Comment 37

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v2)

Looks like I forgot to add some of the test files. Otherwise unchanged.

Comment 38

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

Comment on attachment 8401235 [details] [diff] [review]
Patch to be reviewed (v2)

Unfortunately I'm not going to finish this before I go on PTO.

Comment 39

[Eddy Bruel [:ejpbruel]]

This bug is now mainly about implementing the Worker Debugger API, which is only part of the work we need to make workers debuggable. Please refer to bug 1003097 for the encompassing meta bug.

Comment 40

[Andrew Overholt [:overholt]]

Comment on attachment 8401235 [details] [diff] [review]
Patch to be reviewed (v2)

Since Kyle has been working with Eddy on the design, he's a better reviewer than Ben here.  Also, Ben's queue is giant and even though Kyle is on PTO right now, he'll (most likely) be back before Ben can get to this.

Comment 41

[Eddy Bruel [:ejpbruel]]

(In reply to Andrew Overholt [:overholt] from comment #40)
> Comment on attachment 8401235 [details] [diff] [review]
> Patch to be reviewed (v2)
> 
> Since Kyle has been working with Eddy on the design, he's a better reviewer
> than Ben here.  Also, Ben's queue is giant and even though Kyle is on PTO
> right now, he'll (most likely) be back before Ben can get to this.

Agreed. This isn't blocking me at the moment, so it can wait for Kyle to get back.

Note that I have slightly rebased and updated the patch since I last put it up for review, so I should put up a new version some time soon.

Comment 42

[Eddy Bruel [:ejpbruel]]

I have a rebased and up to date patch that's ready to be uploaded, but if bugzilla is to believed, khuey doesn't exist, so I can't actually set him as reviewer, and since a reviewer is required, that means I can't upload the patch.

Is this because khuey is still on pto? Or is bugzilla being wonky?

Comment 43

[Victor Porof [:vporof][:vp]]

(In reply to Eddy Bruel [:ejpbruel] from comment #42)

Sounds to me like you're in the zone. where normal things. don't happen. very often.

Comment 44

[Andrew Overholt [:overholt]]

Kyle removed his name and ":khuey" while he was away.  Auto-complete should work if you try on just his email address:  khuey ...

Comment 45

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

Yeah I removed the shorthand so people would know that I'm gone.  I'm back now.

Comment 46

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v2)

Rebased and up to date patch

Comment 47

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v3)

This patch was in need of another rebase. I took that opportunity to clean up some parts of the patch, change some of the function names to match the API we ended up with on the debugger server, and to improve the tests.

While improving the tests I ran into some assertion failures while using the sandbox API. It turns out that were caused by the fact that SpiderMonkey can now share atoms between runtimes, and some of the assertions for this were buggy. We never ran into this before because we never used wrappers in workers up until this point, and the problem only manifests for certain atoms.

I've fixed the offending assertions, and all tests seem to be passing now.

Comment 48

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

I'm expecting a new patch here, right?

Comment 49

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #48)
> I'm expecting a new patch here, right?

Yes, sorry about not communicating. I've been working on integrating the worker debugger server with the server on the main thread and discovered several more bugs in the platform API in the process. I'll have a new patch coming up in a minute.

Comment 50

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v4)

I fixed several more bugs related to compartment handling in the presence of more than one global, and error reporting in particular.

I'm now fairly confident that this patch is ready to land, provided it gets a positive review.

Kyle, do you think you'll have time to review this some time soon? I now have several patches that depend on this, so I'm officially blocked on this patch.

Comment 51

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

I can't commit to a review time for this.  Half of my team is on vacation this month and I'm likely going to get caught up in a bunch of 2.0 stabilization work.  This is probably going to wait for some time.

Comment 52

[Rob Campbell [:rc] (:robcee)]

Kyle, this has been sitting in your review queue for over a month now. I realize it's a big patch but it's important stuff. Can you find some way to get this moving?

Comment 53

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v5)

I've rebased the patch once again and also added some code to make debugger timers work properly in the presence of nested event loops.

I also did a push to try to see how much this patch breaks the existing code:
https://tbpl.mozilla.org/?tree=Try&rev=42e810c9b98c

In theory, as long as you don't try to debug any workers, the only observable behavior this patch should change is registering/unregistering each worker on the WorkerDebuggerManager. And of course, that's exactly the part that breaks :-)

The OS X 10.6 Debug Oth failure is interesting because its an assertion failure: a debugger is unregistered before its registered. That should not be possible, so that's probably a race condition.

The tests that complain about window being null instead of undefined are more confusing. Is it possible for this value to be different between platforms somehow?

Comment 54

[Eddy Bruel [:ejpbruel]]

Kyle, would it speed up the review process if I split this patch up into smaller subpatches?

Comment 55

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerManager

In an attempt to move things forward, I've refactored the patch into a series of smaller subpatches. Hopefully, this will make things less daunting to review.

The first patch in the series implements the WorkerDebuggerManager service. Each worker registers a WorkerDebugger object with this service just before it starts running, and unregisters this object again just before it shuts down. The WorkerDebugger object acts as a reference to the worker on the main thread, and exposes the debugger API for that worker.

Comment 56

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerManager

Comment 57

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

Comment on attachment 8482839 [details] [diff] [review]
Implement a WorkerDebuggerManager

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

All the IDL files should be in dom/workers.

I think you need to always go to the main thread here to manipulate mListeners and mDebuggers.  Consider a short-lived subworker.  If there is a listener attached when it registers the debugger it will go to the main thread to enable.  If that listener is removed before the worker unregisters the debugger we will call disable off the main thread, which could be before the message we sent to the main thread runs ...

The enumeration API is also inherently racy, because we have to go to the main thread to fire the listener notifications but the changes are made to the array the enumerator picks from immediately.  If we only touched mListeners and mDebuggers on the main thread it would fix this ...

::: dom/workers/RuntimeService.cpp
@@ +62,5 @@
>  #endif
>  
>  #include "ServiceWorker.h"
>  #include "SharedWorker.h"
> +#include "WorkerDebuggerManager.h"

Seems unlikely that the include is necessary since there are no other changes to this file.

::: dom/workers/WorkerDebuggerManager.cpp
@@ +37,5 @@
> +NS_IMETHODIMP
> +WorkerDebuggerEnumerator::GetNext(nsISupports** aResult)
> +{
> +  if (mIndex == mDebuggers.Length()) {
> +    return NS_ERROR_UNEXPECTED;

Per http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsISimpleEnumerator.idl#41 this should be NS_ERROR_FAILURE.

@@ +53,5 @@
> +}
> +
> +WorkerDebuggerManager::~WorkerDebuggerManager()
> +{
> +  AssertIsOnMainThread();

I'm not convinced that this assertion will hold, since this has threadsafe refcounting.  We should have shutdown workers completely well before we kill the service manager though, so perhaps it does.

@@ +95,5 @@
> +  MutexAutoLock lock(mMutex);
> +
> +  if (!mListeners.Contains(aListener)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

typically removing a non-existent listener fails silently rather than throwing

@@ +105,5 @@
> +void
> +WorkerDebuggerManager::RegisterDebugger(WorkerDebugger* aDebugger)
> +{
> +  bool hasListeners = false;
> +  {

nit: \n before {

@@ +121,5 @@
> +  } else {
> +    nsCOMPtr<nsIRunnable> runnable =
> +       NS_NewRunnableMethodWithArg<nsRefPtr<WorkerDebugger>>(this,
> +         &WorkerDebuggerManager::RegisterDebuggerOnMainThread, aDebugger);
> +    if (NS_FAILED(NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL))) {

This should only ever fail at shutdown, well after workers are shutdown, so go ahead and MOZ_ALWAYS_TRUE(NS_SUCCEDED(NS_DispatchToMainThread ...))

@@ +132,5 @@
> +void
> +WorkerDebuggerManager::UnregisterDebugger(WorkerDebugger* aDebugger)
> +{
> +  bool hasListeners = false;
> +  {

nit: \n before {

@@ +141,5 @@
> +    hasListeners = !mListeners.IsEmpty();
> +  }
> +
> +  if (!hasListeners) {
> +    aDebugger->Disable();

Same here.

@@ +148,5 @@
> +  } else {
> +    nsCOMPtr<nsIRunnable> runnable =
> +       NS_NewRunnableMethodWithArg<nsRefPtr<WorkerDebugger>>(this,
> +         &WorkerDebuggerManager::UnregisterDebuggerOnMainThread, aDebugger);
> +    if (NS_FAILED(NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL))) {

here too

@@ +163,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  nsTArray<nsCOMPtr<nsIWorkerDebuggerManagerListener>> listeners;
> +  {

nit: \n before {

@@ +182,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  nsTArray<nsCOMPtr<nsIWorkerDebuggerManagerListener>> listeners;
> +  {

nit: \n before

::: dom/workers/WorkerPrivate.cpp
@@ +761,5 @@
>  
>      RuntimeService* runtime = RuntimeService::GetService();
>      NS_ASSERTION(runtime, "This should never be null!");
>  
> +    mFinishedWorker->DisableDebugger();

Please be consistent about the ordering of DisableDebugger and UnregisterWorker.

@@ +799,5 @@
>      JSAutoRequest ar(cx);
>  
>      runtime->UnregisterWorker(cx, mFinishedWorker);
>  
> +    mFinishedWorker->DisableDebugger();

e.g. Here it comes after unregistering ...  It should almost certainly come before.

@@ +3612,5 @@
> +  mCondVar.Notify();
> +}
> +
> +void
> +WorkerDebugger::Enable()

There should be assertions that we only call this once.

@@ +3620,5 @@
> +  NotifyIsEnabled(true);
> +}
> +
> +void
> +WorkerDebugger::Disable()

This too.

::: dom/workers/WorkerPrivate.h
@@ +305,4 @@
>    ClearSelfRef()
>    {
>      AssertIsOnParentThread();
> +

Extraneous \n addition.

@@ +725,5 @@
>    { }
>  #endif
>  };
>  
> +class WorkerDebugger : public nsIWorkerDebugger {

Move this to the .cpp and just forward declare it here.

@@ +727,5 @@
>  };
>  
> +class WorkerDebugger : public nsIWorkerDebugger {
> +  mozilla::Mutex mMutex;
> +  mozilla::CondVar mCondVar;

You don't have to, but you could just use mozilla::Monitor here, which is a combination of a mutex and a condvar.

::: layout/build/nsLayoutModule.cpp
@@ +1014,5 @@
>    { &kNS_TEXTEDITOR_CID, false, nullptr, nsPlaintextEditorConstructor },
>    { &kDOMREQUEST_SERVICE_CID, false, nullptr, DOMRequestServiceConstructor },
>    { &kQUOTA_MANAGER_CID, false, nullptr, QuotaManagerConstructor },
>    { &kSERVICEWORKERMANAGER_CID, false, nullptr, ServiceWorkerManagerConstructor },
> +  { &kWORKERDEBUGGERMANAGER_CID, false, nullptr, WorkerDebuggerManagerConstructor },

the second value here should be true to indicate that its a service (as you can see, we often forget that ...)

Comment 58

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerManager (v2)

New patch with comments addressed.

Comment 59

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebugger

This patch adds several properties to the nsIWorkerDebugger interface which are intended to filter workers by window, parent, etc.

Comment 60

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

Comment on attachment 8483414 [details] [diff] [review]
Implement a WorkerDebuggerManager (v2)

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

r- mostly due to the volume of comments.  This looks pretty good, just want to see it again.  I think the only open design issue here is the "not blocking all the time" thing I mention.

::: dom/workers/WorkerDebuggerManager.cpp
@@ +1,1 @@
> +#include "WorkerDebuggerManager.h"

This file needs a license header.

@@ +61,5 @@
> +
> +NS_IMETHODIMP
> +WorkerDebuggerManager::GetWorkerDebuggerEnumerator(
> +                                                  nsISimpleEnumerator** aResult)
> +{

Will this ever need to be called off the main thread?  I suspect not.  If that's the case, AssertIsOnMainThread() here.

If that's true, you also don't need the mutex to protect mDebuggers.

@@ +115,5 @@
> +        &WorkerDebuggerManager::RegisterDebuggerOnMainThread, aDebugger);
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> +      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL)));
> +
> +    aDebugger->WaitIsEnabled(true);

Having to do this even when the debugger is not active isn't particularly awesome.  Can we check mListeners here and only block if there are listeners?

That makes this a little racy, in that if you add a listener in the middle of the registration process (between here and the runnable running on the main thread) we won't wait for it.  I think we can live with that to avoid blocking in the 99% of the time the debugger is not in use.

@@ +133,5 @@
> +        &WorkerDebuggerManager::UnregisterDebuggerOnMainThread, aDebugger);
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> +      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL)));
> +
> +    aDebugger->WaitIsEnabled(false);

here too.

::: dom/workers/WorkerDebuggerManager.h
@@ +1,1 @@
> +#ifndef mozilla_dom_workers_workerdebuggermanager_h

This one too.

::: dom/workers/WorkerPrivate.cpp
@@ +3616,5 @@
> +
> +void
> +WorkerDebugger::Enable()
> +{
> +  // May be called on any thread!

This isn't true anymore, though.  Unless this is going to change in a later patch, assert main thread only here.  That also means WaitIsEnabled should probably assert that it's *not* on the main thread.

@@ +3628,5 @@
> +
> +void
> +WorkerDebugger::Disable()
> +{
> +  // May be called on any thread!

And here.

::: dom/workers/WorkerPrivate.h
@@ +724,5 @@
>    { }
>  #endif
>  };
>  
> +class WorkerDebugger : public nsIWorkerDebugger {

Move this to the .cpp and forward declare it here.

::: dom/workers/moz.build
@@ +72,5 @@
>      'XMLHttpRequest.cpp',
>      'XMLHttpRequestUpload.cpp',
>  ]
>  
> +

extraneous \n

Comment 61

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

Comment on attachment 8483414 [details] [diff] [review]
Implement a WorkerDebuggerManager (v2)

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

::: dom/workers/WorkerDebuggerManager.cpp
@@ +133,5 @@
> +        &WorkerDebuggerManager::UnregisterDebuggerOnMainThread, aDebugger);
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> +      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL)));
> +
> +    aDebugger->WaitIsEnabled(false);

So actually, I think we need to keep this one unconditionally to ensure that the debugger drops mWorkerPrivate before the WorkerPrivate is GCd.  Blocking when shutting down isn't as bad as blocking when starting up though, so I think this is fine.

Comment 62

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

Comment on attachment 8483416 [details] [diff] [review]
Implement a WorkerDebugger

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

I didn't look at this too closely, since it'll need to be rebased over the changes to the previous patch.

::: dom/workers/WorkerPrivate.cpp
@@ +2266,5 @@
> +  nsCOMPtr<nsIThread> mainThread;
> +  if (NS_FAILED(NS_GetMainThread(getter_AddRefs(mainThread))) ||
> +      NS_FAILED(NS_ProxyRelease(mainThread, self->mDebugger))) {
> +    NS_WARNING("Failed to proxy release of debugger, leaking instead!");
> +  }

Why do you need this?  The debugger has threadsafe refcounting, no?  If it's just to deal with the listener array you should proxy that instead, from the WorkerDebugger's dtor.  Unless you're absolutely certain that this is the last reference this doesn't guarantee that the dtor runs on the main thread anyways.

@@ +3619,5 @@
> +  }
> +
> +  MOZ_ASSERT(mWorkerPrivate->IsDedicatedWorker());
> +
> +  nsRefPtr<nsIWorkerDebugger> debugger = parent->Debugger();

nsCOMPtr

@@ +3670,5 @@
> +    *aResult = nullptr;
> +    return NS_OK;
> +  }
> +
> +  nsRefPtr<nsPIDOMWindow> window = mWorkerPrivate->GetWindow();

nsCOMPtr

@@ +3749,5 @@
> +    nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewRunnableMethod(this, &WorkerDebugger::DisableOnMainThread);
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> +      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL)));
> +  }

Based on the comments on the last patch I think this is all unnecessary.

::: dom/workers/WorkerPrivate.h
@@ +737,5 @@
>  
>    // Protected by mMutex
>    WorkerPrivate* mWorkerPrivate;
>    bool mIsEnabled;
> +  nsTArray<nsCOMPtr<nsIWorkerDebuggerListener>> mListeners;

This doesn't need to be protected by a lock.  It's only touched on the main thread.

::: dom/workers/nsIWorkerDebugger.idl
@@ +12,5 @@
>  interface nsIWorkerDebugger : nsISupports
>  {
> +  const unsigned long TYPE_DEDICATED = 0;
> +  const unsigned long TYPE_SHARED = 1;
> +  const unsigned long TYPE_SERVICE = 2;

Add a comment to the WorkerType enum declaration saying that this list must be updated too.

Comment 63

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #60)
> Comment on attachment 8483414 [details] [diff] [review]
> Implement a WorkerDebuggerManager (v2)
> 
> Review of attachment 8483414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +115,5 @@
> > +        &WorkerDebuggerManager::RegisterDebuggerOnMainThread, aDebugger);
> > +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> > +      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL)));
> > +
> > +    aDebugger->WaitIsEnabled(true);
> 
> Having to do this even when the debugger is not active isn't particularly
> awesome.  Can we check mListeners here and only block if there are listeners?
> 
> That makes this a little racy, in that if you add a listener in the middle
> of the registration process (between here and the runnable running on the
> main thread) we won't wait for it.  I think we can live with that to avoid
> blocking in the 99% of the time the debugger is not in use.

If we ever wanted to implement a 'pause on start' feature for workers, we need some guarantee that when the worker is registered on the main thread it hasn't started running yet (so we can set up our debugger at that time). Making this racy would make that invariant unreliable, and thus unusable.

How about this instead: we check if there are any listeners at the time of the call to RegisterWorkerDebugger, and pass the result to the runnable that calls RegisterWorkerDebuggerOnMainThread. If there were no listeners, we won't wait for the main thread in RegisterWorkerDebugger, but we *also* won't call any listeners in RegisterWorkerDebuggerOnMainThread, not even if you add one while this runnable is on the queue.

Pretty much agree with everything else. Let me know what you want to do here, so I can put the next version of the patch up.

Comment 64

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

(In reply to Eddy Bruel [:ejpbruel] from comment #63)
> How about this instead: we check if there are any listeners at the time of
> the call to RegisterWorkerDebugger, and pass the result to the runnable that
> calls RegisterWorkerDebuggerOnMainThread. If there were no listeners, we
> won't wait for the main thread in RegisterWorkerDebugger, but we *also*
> won't call any listeners in RegisterWorkerDebuggerOnMainThread, not even if
> you add one while this runnable is on the queue.

Yeah, that sounds great.  Wish I had thought of it myself!

Comment 65

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerManager (v3)

New patch with comments addressed.

Note that I cannot move the definition of WorkerDebugger from WorkerPrivate.h to WorkerPrivate.cpp, because we access it from WorkerDebuggerManager.cpp.

Comment 66

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebugger (v2)

New patch with comments addressed.

Comment 67

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerGlobalScope

This patch implements the global object that will be used by the debugger.

Note that this is the only patch that doesn't include tests, because we won't be able to access this object until the next patch.

Comment 68

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebugger.initialize

This patch implements WorkerDebugger.initialize, which allows you to initialize the debugger by loading its initial script.

Comment 69

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

Comment on attachment 8484957 [details] [diff] [review]
Implement a WorkerDebuggerManager (v3)

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

::: dom/workers/WorkerDebuggerManager.cpp
@@ +50,5 @@
> +  element.forget(aResult);
> +  return NS_OK;
> +};
> +
> +class RegisterDebuggerRunnable MOZ_FINAL : public nsIRunnable

inherit nsRunnable please

@@ +102,5 @@
> +
> +  MutexAutoLock lock(mMutex);
> +
> +  nsRefPtr<WorkerDebuggerEnumerator> enumerator =
> +    new WorkerDebuggerEnumerator(mDebuggers);

mDebuggers is only ever touched on the main thread, so we don't need a lock here, or anywhere we touch it.

::: dom/workers/WorkerPrivate.cpp
@@ +3595,5 @@
> +}
> +
> +void
> +WorkerDebugger::WaitIsEnabled(bool aIsEnabled)
> +{

assert that you're not on the main thread.

Comment 70

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

Comment on attachment 8484958 [details] [diff] [review]
Implement a WorkerDebugger (v2)

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

You didn't address my comments ...

Comment 71

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

Comment on attachment 8484959 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope

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

::: dom/webidl/WorkerDebuggerGlobalScope.webidl
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +[Global=(Worker), Exposed=Worker]
> +interface WorkerDebuggerGlobalScope : EventTarget {
> +  readonly attribute WorkerGlobalScope global;

So what's the security model here?  Workers don't provide xrays/etc, so there's no guarantee that any of the methods/properties on global are actually sane.  We need a default-deny security policy here.  Do we get that by default?

::: dom/workers/WorkerPrivate.h
@@ +1093,5 @@
>    MessagePort*
>    GetMessagePort(uint64_t aMessagePortSerial);
>  
> +  WorkerGlobalScope*
> +  GetOrCreateGlobalScope(JSContext* aCx);

*grumble*

Add a comment saying that you should use GlobalScope() unless it's possible that the global does not already exist.

::: dom/workers/WorkerScope.cpp
@@ +404,5 @@
> +  return mWorkerPrivate->GetOrCreateGlobalScope(aCx);
> +}
> +
> +void
> +WorkerDebuggerGlobalScope::Dump(const nsAString& aString) const

You really should just call Dump on Global().

Comment 72

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

Comment on attachment 8484960 [details] [diff] [review]
Implement WorkerDebugger.initialize

I'm going to wait for the rest of the set to be updated before looking at this.

Comment 73

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebugger (v3)

This time with comments actually addressed.

Comment 74

[Eddy Bruel [:ejpbruel]]

I'm at the devtools workweek right now. After the workweek, I will be on PTO for two weeks to celebrate my honeymoon. It would be awesome if we could do a review sprint this week, so I can land as many of these patches as possible before I leave. I would like to avoid having to unbitrot all of them when I get back.

Comment 75

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

Comment on attachment 8494725 [details] [diff] [review]
Implement a WorkerDebugger (v3)

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

::: dom/workers/WorkerPrivate.cpp
@@ +3607,5 @@
> +
> +  nsCOMPtr<nsIThread> mainThread;
> +  if (NS_FAILED(NS_GetMainThread(getter_AddRefs(mainThread)))) {
> +    NS_WARNING("Failed to proxy release of listeners, leaking instead!");
> +  }

You should guard this all in an if (!NS_IsMainThread()) ...

@@ +3611,5 @@
> +  }
> +
> +  for (size_t index = 0; index < mListeners.Length(); ++index) {
> +    if (NS_FAILED(NS_ProxyRelease(mainThread, mListeners[index]))) {
> +      NS_WARNING("Failed to proxy release of listener, leaking instead!");

If this fails the relevant entry in mListeners is going remain there, so you're going to Release() it on this thread, not leak it.

@@ +3663,5 @@
> +  }
> +
> +  MOZ_ASSERT(mWorkerPrivate->IsDedicatedWorker());
> +
> +  nsCOMPtr<nsIWorkerDebugger> debugger = parent->Debugger();

MOZ_ASSERT(debugger)?

@@ +3792,5 @@
> +    MutexAutoUnlock unlock(mMutex);
> +
> +    for (size_t index = 0; index < mListeners.Length(); ++index) {
> +      mListeners[index]->OnClose();
> +    }

We should probably do something sane here to handle listeners that remove themselves or their friends from the array.  Perhaps mListeners should be an nsTObserverArray?

Comment 76

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #75)
> Comment on attachment 8494725 [details] [diff] [review]
> Implement a WorkerDebugger (v3)
> 
> Review of attachment 8494725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3607,5 @@
> > +
> > +  nsCOMPtr<nsIThread> mainThread;
> > +  if (NS_FAILED(NS_GetMainThread(getter_AddRefs(mainThread)))) {
> > +    NS_WARNING("Failed to proxy release of listeners, leaking instead!");
> > +  }
> 
> You should guard this all in an if (!NS_IsMainThread()) ...
> 
> @@ +3611,5 @@
> > +  }
> > +
> > +  for (size_t index = 0; index < mListeners.Length(); ++index) {
> > +    if (NS_FAILED(NS_ProxyRelease(mainThread, mListeners[index]))) {
> > +      NS_WARNING("Failed to proxy release of listener, leaking instead!");
> 
> If this fails the relevant entry in mListeners is going remain there, so
> you're going to Release() it on this thread, not leak it.
> 
> @@ +3663,5 @@
> > +  }
> > +
> > +  MOZ_ASSERT(mWorkerPrivate->IsDedicatedWorker());
> > +
> > +  nsCOMPtr<nsIWorkerDebugger> debugger = parent->Debugger();
> 
> MOZ_ASSERT(debugger)?
> 
> @@ +3792,5 @@
> > +    MutexAutoUnlock unlock(mMutex);
> > +
> > +    for (size_t index = 0; index < mListeners.Length(); ++index) {
> > +      mListeners[index]->OnClose();
> > +    }
> 
> We should probably do something sane here to handle listeners that remove
> themselves or their friends from the array.  Perhaps mListeners should be an
> nsTObserverArray?

Thanks Kyle! I'll have new patches up tomorrow.

Comment 77

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #75)

I have new patches that are about ready for review, but I have some questions about the use of NS_ProxyRelease before I can finish them up. See comments below.

> Comment on attachment 8494725 [details] [diff] [review]
> Implement a WorkerDebugger (v3)
> 
> Review of attachment 8494725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3607,5 @@
> > +
> > +  nsCOMPtr<nsIThread> mainThread;
> > +  if (NS_FAILED(NS_GetMainThread(getter_AddRefs(mainThread)))) {
> > +    NS_WARNING("Failed to proxy release of listeners, leaking instead!");
> > +  }
> 
> You should guard this all in an if (!NS_IsMainThread()) ...
> 
> @@ +3611,5 @@
> > +  }
> > +
> > +  for (size_t index = 0; index < mListeners.Length(); ++index) {
> > +    if (NS_FAILED(NS_ProxyRelease(mainThread, mListeners[index]))) {
> > +      NS_WARNING("Failed to proxy release of listener, leaking instead!");
> 
> If this fails the relevant entry in mListeners is going remain there, so
> you're going to Release() it on this thread, not leak it.
> 

Since listeners aren't thread safe, and always created on the main thread, wouldn't releasing them off the main thread cause us to crash? So aren't we *supposed* to leak them in case proxying the release fails? If so, what's the preferred way to do that? If not, what am I missing?

> @@ +3663,5 @@
> > +  }
> > +
> > +  MOZ_ASSERT(mWorkerPrivate->IsDedicatedWorker());
> > +
> > +  nsCOMPtr<nsIWorkerDebugger> debugger = parent->Debugger();
> 
> MOZ_ASSERT(debugger)?
> 

I'll to put this assert in the Debugger() accessor instead.

> @@ +3792,5 @@
> > +    MutexAutoUnlock unlock(mMutex);
> > +
> > +    for (size_t index = 0; index < mListeners.Length(); ++index) {
> > +      mListeners[index]->OnClose();
> > +    }
> 
> We should probably do something sane here to handle listeners that remove
> themselves or their friends from the array.  Perhaps mListeners should be an
> nsTObserverArray?

The simplest solution is probably to create a copy of the listener array and iterate over that (as we do in the WorkerDebuggerManager). nsIObserver seems like overkill to me.

Please let me know what I'm supposed to do with NS_ProxyRelease, so I can finish up the patch. I can put them up either in the next hour or after the team dinner.

Comment 78

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

(In reply to Eddy Bruel [:ejpbruel] from comment #77)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #75)
> Since listeners aren't thread safe, and always created on the main thread,
> wouldn't releasing them off the main thread cause us to crash? So aren't we
> *supposed* to leak them in case proxying the release fails? If so, what's
> the preferred way to do that? If not, what am I missing?

Yes, that is the preferred thing to do, but that's not what you're doing.  To leak then you would need to do:

for (size_t index = 0; index < mListeners.Length(); ++index) {
  Foo* listener;
  mListeners[index].forget(&listener);
  if (NS_FAILED(NS_ProxyRelease(mainThread, listener))) {
    NS_WARNING("Failed to proxy release of listener, leaking instead!");
  }
}

> The simplest solution is probably to create a copy of the listener array and
> iterate over that (as we do in the WorkerDebuggerManager). nsIObserver seems
> like overkill to me.

nsTObserverArray doesn't require using nsIObserver, but it effectively makes a copy internally, so either is fine.

Comment 79

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a WorkerDebuggerGlobalScope (v2)

I noticed that you actually already r+'d the previous patch, so I will land that with the requested changes applied.

This patch mainly adds a security policy for accessing the worker's global to the debugger. I'm flagging bholley for those changes. Note that we talked about debugger sandboxes on irc, but this patch doesn't take those into account yet. They will be implemented in a future patch.

Flagging khuey for the other stuff.

Comment 80

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebugger.initialize

This patch implements WorkerDebugger.initialize, which creates the debugger global and loads the main debugger script in it.

Comment 81

[Bobby Holley (:bholley)]

Comment on attachment 8498506 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope (v2)

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

I would like us all to take a moment to appreciate the number of giants whose shoulders this patch stands on. Seriously. A patch this simple would have been unthinkable three years ago, before CPG, WebIDL bindings, and OMT CC.

r=bholley with those things addressed.

::: dom/webidl/WorkerDebuggerGlobalScope.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +[Global=(Worker), Exposed=Worker]
> +interface WorkerDebuggerGlobalScope : EventTarget {

Does this actually need to be an EventTarget?

::: dom/workers/RuntimeService.cpp
@@ +865,5 @@
> +
> +const OpaqueDebuggerWrapper OpaqueDebuggerWrapper::singleton;
> +
> +JSObject*
> +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,

Give this a better name - SelectWorkerSecurityWrapper.

@@ +868,5 @@
> +JSObject*
> +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,
> +     JS::HandleObject parent)
> +{
> +  JSObject* global = JS::CurrentGlobalOrNull(cx);

Call this "targetGlobal".

@@ +869,5 @@
> +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,
> +     JS::HandleObject parent)
> +{
> +  JSObject* global = JS::CurrentGlobalOrNull(cx);
> +  if (!IsDebuggerGlobal(global)) {

Don't we also need to check for debugger sandboxes here? Or are those just additional DebuggerGlobals?

@@ +870,5 @@
> +     JS::HandleObject parent)
> +{
> +  JSObject* global = JS::CurrentGlobalOrNull(cx);
> +  if (!IsDebuggerGlobal(global)) {
> +    MOZ_CRASH();

Please add a string argument that says something like "There should be no edges from the worker scope to the debugger scope").

@@ +878,5 @@
> +  if (IsWorkerGlobal(obj)) {
> +    wrapper = &OpaqueDebuggerWrapper::singleton;
> +  } else {
> +    wrapper = &js::CrossCompartmentWrapper::singleton;
> +  }

This fails in an unsafe manner if we ever accidentally expose a non-global. Please make this do the following:

JSObject *originGlobal = js::GetGlobalForObjectCrossCompartment(obj);
if (IsDebuggerGlobal(originGlobal)) {
  wrapper = &js::CrossCompartmentWrapper::singleton;
} else {
  MOZ_RELEASE_ASSERT(IsWorkerGlobal(originGlobal));
  if (obj == originGlobal) {
    wrapper = &OpaqueDebuggerWrapper::singleton;
  } else {
    MOZ_CRASH("The only debugger->debugee edge should be to the global");
  }
}

::: dom/workers/WorkerScope.cpp
@@ +372,5 @@
>                                                         true);
>  }
>  
> +WorkerDebuggerGlobalScope::WorkerDebuggerGlobalScope(
> +                                                  WorkerPrivate* aWorkerPrivate)

Indentation here looks weird.

@@ +420,5 @@
> +  return mWorkerPrivate->GetOrCreateGlobalScope(aCx)->Dump(aString);
> +}
> +
> +nsIGlobalObject*
> +GetGlobalObjectForGlobal(JSObject* global)

Hm, this name kind of sucks. Also, is there any reason we can't just use xpc::NativeGlobal for this? I'm happy to move it somewhere out of XPConnect if the machinery does what we need.

Comment 82

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #81)
> Comment on attachment 8498506 [details] [diff] [review]
> Implement a WorkerDebuggerGlobalScope (v2)
> 
> Review of attachment 8498506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like us all to take a moment to appreciate the number of giants
> whose shoulders this patch stands on. Seriously. A patch this simple would
> have been unthinkable three years ago, before CPG, WebIDL bindings, and OMT
> CC.
> 
> r=bholley with those things addressed.
> 
> ::: dom/webidl/WorkerDebuggerGlobalScope.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +[Global=(Worker), Exposed=Worker]
> > +interface WorkerDebuggerGlobalScope : EventTarget {
> 
> Does this actually need to be an EventTarget?
> 

Yes. I have a future patch that will emit message events on the global scope.

> ::: dom/workers/RuntimeService.cpp
> @@ +865,5 @@
> > +
> > +const OpaqueDebuggerWrapper OpaqueDebuggerWrapper::singleton;
> > +
> > +JSObject*
> > +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,
> 
> Give this a better name - SelectWorkerSecurityWrapper.
> 
> @@ +868,5 @@
> > +JSObject*
> > +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,
> > +     JS::HandleObject parent)
> > +{
> > +  JSObject* global = JS::CurrentGlobalOrNull(cx);
> 
> Call this "targetGlobal".
> 
> @@ +869,5 @@
> > +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj,
> > +     JS::HandleObject parent)
> > +{
> > +  JSObject* global = JS::CurrentGlobalOrNull(cx);
> > +  if (!IsDebuggerGlobal(global)) {
> 
> Don't we also need to check for debugger sandboxes here? Or are those just
> additional DebuggerGlobals?
> 

Yes, but like I said in the comments, sandboxes aren't implemented by this patch, so I can't check for them yet. I have a future patch that implements debugger sandboxes and adds an extra check for them here.

> @@ +870,5 @@
> > +     JS::HandleObject parent)
> > +{
> > +  JSObject* global = JS::CurrentGlobalOrNull(cx);
> > +  if (!IsDebuggerGlobal(global)) {
> > +    MOZ_CRASH();
> 
> Please add a string argument that says something like "There should be no
> edges from the worker scope to the debugger scope").
> 
> @@ +878,5 @@
> > +  if (IsWorkerGlobal(obj)) {
> > +    wrapper = &OpaqueDebuggerWrapper::singleton;
> > +  } else {
> > +    wrapper = &js::CrossCompartmentWrapper::singleton;
> > +  }
> 
> This fails in an unsafe manner if we ever accidentally expose a non-global.
> Please make this do the following:
> 
> JSObject *originGlobal = js::GetGlobalForObjectCrossCompartment(obj);
> if (IsDebuggerGlobal(originGlobal)) {
>   wrapper = &js::CrossCompartmentWrapper::singleton;
> } else {
>   MOZ_RELEASE_ASSERT(IsWorkerGlobal(originGlobal));
>   if (obj == originGlobal) {
>     wrapper = &OpaqueDebuggerWrapper::singleton;
>   } else {
>     MOZ_CRASH("The only debugger->debugee edge should be to the global");
>   }
> }
> 
> ::: dom/workers/WorkerScope.cpp
> @@ +372,5 @@
> >                                                         true);
> >  }
> >  
> > +WorkerDebuggerGlobalScope::WorkerDebuggerGlobalScope(
> > +                                                  WorkerPrivate* aWorkerPrivate)
> 
> Indentation here looks weird.
> 

It's consistent with the indentation used elsewhere.

> @@ +420,5 @@
> > +  return mWorkerPrivate->GetOrCreateGlobalScope(aCx)->Dump(aString);
> > +}
> > +
> > +nsIGlobalObject*
> > +GetGlobalObjectForGlobal(JSObject* global)
> 
> Hm, this name kind of sucks. Also, is there any reason we can't just use
> xpc::NativeGlobal for this? I'm happy to move it somewhere out of XPConnect
> if the machinery does what we need.

I'm not sure. I'll look into it.

Comment 83

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

Comment on attachment 8498506 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope (v2)

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

r=me with bholley's changes

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5fcb5f05f03

Comment 85

[Eddy Bruel [:ejpbruel]]

Oops. This should have been left open.

Comment 86

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebugger.postMessage

Comment 87

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebuggerGlobalScope.reportError

Comment 88

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebuggerGlobalScope.enterEventLoop

Comment 89

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e30de6729aff

Comment 90

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

Please close this bug and open a new one for the new things.  This bug has nearly 100 comments, we don't need to try to track even more things here.

Comment 91

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e30de6729aff

Comment 92

[Eddy Bruel [:ejpbruel]]

I've opened a new bug for the remainder of the work, see bug 1092102.

Comment 93

[Panos Astithas (he/him) [:past] (please ni?)]

The status should probably be FIXED instead of INCOMPLETE, as there was significant work that landed in this bug. INCOMPLETE is usually used for non-actionable or just plain useless bug reports.

That said, I don't know if this will affect anyone's search queries or not, so feel free to revert if you disagree.

Comment 94

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Eddy, check if the bugs that were blocked by this one are also blocked by the continuing work.  I tried to move a few, but wasn't sure about the others.