1241965 - Workers should not start running until registration is complete.

Status: RESOLVED DUPLICATE of bug 1527203

(DevTools :: Debugger, defect, P2)

Description

[Eddy Bruel [:ejpbruel]]

For certain features, we need to be able to initialise a debugger server in a worker before it starts running. To implement this, we want the registration callback on the WorkerDebuggerManager to return a Promise, and prevent the worker from starting until that Promise is resolved.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed solution (WIP)

Here is a proposed solution that I think should do the trick (still needs test obv.). The idea is to pass a callback as the second argument to the onRegister handler. The onRegister handler should call this callback when it has finished its work.

Until all onRegister handlers have called the callback, the parent thread will remain block. This prevents the CompileScriptRunnable from being dispatched, so the main worker script will not start running until registration is complete. Note, however, that it does not prevent the worker from spinning its main run loop, so we can still do things like initialize a debugger, post messages to its, etc. during registration.

Kyle, what do you think about this approach? This code would still need tests, of course.

Comment 2

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

Comment on attachment 8734424 [details] [diff] [review]
Proposed solution (WIP)

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

It looks generally sane, yes.

::: dom/workers/WorkerDebuggerManager.cpp
@@ +356,5 @@
> +      aWorkerPrivate->SetIsDebuggerRegistered(true);
> +    } else {
> +      nsCOMPtr<nsIWorkerDebuggerManagerListenerCallback> callback =
> +        new WorkerDebuggerManagerListenerCallback(aWorkerPrivate,
> +                                                  listeners.Length());

I would write this as

if (aNotifyListeners) {
  // Get Listeners

  if (!listeners.IsEmpty()) {
    // call Callbacks
    return;
  }
}

// SetIsDebuggerRegistered(true)

::: dom/workers/nsIWorkerDebuggerManager.idl
@@ +3,5 @@
>  interface nsISimpleEnumerator;
>  interface nsIWorkerDebugger;
>  
> +[scriptable, function, uuid(ae5c14ae-9f38-43d3-A4dc-62275db5338c)]
> +interface nsIWorkerDebuggerManagerListenerCallback  : nsISupports

nsIWorkerDebuggerRegistrationCallback is a bit less of a mouthful.

Comment 3

[Eddy Bruel [:ejpbruel]]

Well, as usual, this turns out to be harder than expected.

A worker is always registered from the parent thread. For non-top-level workers, the parent thread dispatches a runnable to the main thread, and then waits for a condition variable to be notified. The main thread creates a debugger for the worker, and registers it so that it can be found later. It then calls the onRegister handler on the listeners (if any), where we can do required initialisation (such as loading a debugger server and setting breakpoints) before the worker starts running.

The onRegister handler is passed a callback, which should be called when initialisation is complete. This allows initialisation to happen asynchronously, which is the point of this bug. When all onRegister handlers have called their callback, the main thread notifies the condition variable, and the parent continues running. Immediately after that, the parent dispatches a CompileScriptRunnable to the worker thread, so that it can start running the main worker script. So far, so good.

For top-level workers, however, the parent thread *is* the main thread. In this case, we currently do not wait for a condition variable at all. We simply call RegisterDebuggerOnMainThread directly (without using a runnable), and assume we are fully initialised when that function returns. We want initialisation to be to happen asynchronously in this case as well, but we don't want to block the main thread (since we still want events to be handled while we are waiting for initialisation to complete).

What I think we should do instead is this:

1. RegisterWorkerDebugger takes a callback as its second argument:
   https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerDebuggerManager.h#80
2. When all onRegister handlers have called *their* callback, WorkerDebuggerManager::RegisterDebugger 
   calls this callback:
   https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerDebuggerManager.cpp#246

   On the main thread, this happens when RegisterDebuggerMainThread returns. On other threads, this
   happens right after the condition variable is notified.
3. In WorkerPrivate.cpp, we use the callback passed to RegisterWorkerDebugger to delay dispatching the
   CompileScriptRunnable until registration is complete:
   https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?from=WorkerPrivate.cpp#4122

   (Note that worker->EnableDebugger() directly calls RegisterWorkerDebugger)

Kyle, what do you think of this solution? Do you have any better ideas?

Comment 4

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

I think I answered this question on IRC.

Comment 5

[Eddy Bruel [:ejpbruel]]

I'm going to implement this bug in three steps:

1. Do some generic cleanup in WorkerDebuggerManager.
2. Add an optional WorkerDebuggerRegisterCallback to WorkerDebuggerManager::RegisterWorker. This callback 
   will be called when registration is complete. It will initially be called immediately after all 
   listeners have been called (but see step 3)
3. Add an nsIWorkerDebuggerRegisterCallback to nsIWorkerDebuggerManagerListener::OnRegister. This callback
   should be called when the when the listener has completed whatever it needs to do before the worker
   starts running. Instead of immediately, the WorkerDebuggerRegisterCallback from step 2 will now be
   called once all listeners have called their nsIWorkerDebuggerRegisterCallback.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Step 1/3 - Do some generic cleanup in WorkerDebuggerManager

Nothing shocking here, just added some useful typedefs, and merged Enable/DisableDebugger and Register/UnregisterWorkerDebugger into a single function.

While we're at it, I don't think the assumption that WorkerDebuggerManager always exists if were calling Register/UnregisterWorkerDebugger from a worker thread holds, so we should just always call GetOrCreate instead of Get.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Step 2/3 - Implement WorkerDebuggerRegisterCallback.

This patch is somewhat more involved. The basic idea is that the callback we pass to RegisterWorker should always be called from the parent thread. If the parent thread is the main thread, we simply call RegisterWorkerMainThread directly, and pass it the callback as argument. Once registration is complete, RegisterWorkerMainThread will call the callback for us.

On the other hand, if the parent is not the main thread, we dispatch a runnable to the main thread to call RegisterWorkerMainThread for us. This time, we pass a different callback, which notifies the condition variable that the parent thread is waiting on. When the parent thread is notified, it calls the callback directly.

As a final note, the callback should always be called, even if registration fails (because the WorkerDebuggerManager is not available), since we still want the worker to start running in that case.

Comment 8

[Eddy Bruel [:ejpbruel]]

I'll finish the patch to implement the final step once I've gotten some feedback on the first two.

Comment 9

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

Comment on attachment 8736377 [details] [diff] [review]
Step 1/3 - Do some generic cleanup in WorkerDebuggerManager

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

::: dom/workers/WorkerDebuggerManager.h
@@ +30,3 @@
>  class WorkerDebugger;
>  
> +typedef nsTArray<RefPtr<WorkerDebugger>> WorkerDebuggers;

Is there a reason not to put these typedefs inside the class?

Comment 10

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

Comment on attachment 8736378 [details] [diff] [review]
Step 2/3 - Implement WorkerDebuggerRegisterCallback.

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

r- for lifetime management issues.

::: dom/workers/WorkerDebuggerManager.cpp
@@ +27,5 @@
> +  RegisterDebuggerMainThreadRunnable(
> +                                                  WorkerPrivate* aWorkerPrivate,
> +                                                          bool aNotifyListeners,
> +                                   WorkerDebuggerRegisterCallbackFunc aCallback,
> +                                                                 void* aClosure)

Please don't do this, this style is *terrible* :)

::: dom/workers/WorkerPrivate.cpp
@@ +4131,5 @@
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>      return nullptr;
>    }
>  
> +  worker->RegisterDebugger(&Start, worker);

We need to increment the busy count or something here (and then decrement it when Start is called, presumably).  If I create a new Worker and the Start() call is pending, what stops the worker from being GC'd if the JS isn't holding it alive.

We should probably write a test for this too.

::: dom/workers/WorkerPrivate.h
@@ +243,5 @@
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(WorkerPrivateParent,
>                                                           DOMEventTargetHelper)
>  
>    void
> +  RegisterDebugger(WorkerDebuggerRegisterCallbackFunc aCallback = nullptr,

Can you just forward declare this so we don't have to pull in WorkerDebuggerManager.h all over the code base?

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Brian: is this something we should take a look at atm? If not, we should probably change to P3

Honza

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #11)

@Brian: is this something we should take a look at atm? If not, we should probably change to P3

Honza

We're going to need this or something like this soon, in order to install breakpoints before the worker starts running. Right now this is blocked on bug 1524374, but after that lands this will be the next step.

Comment 13

[Brian Hackett [Laid off!]]

I'm going to work on this in bug 1527203.