Closed
Bug 1241965
Opened 9 years ago
Closed 5 years ago
Workers should not start running until registration is complete.
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1527203
People
(Reporter: ejpbruel, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
9.17 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
11.87 KB,
patch
|
khuey
:
review-
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Blocks: dbg-worker
Reporter | ||
Comment 1•8 years ago
|
||
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.
Assignee: nobody → ejpbruel
Attachment #8734424 -
Flags: feedback?(khuey)
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.
Attachment #8734424 -
Flags: feedback?(khuey) → feedback+
Reporter | ||
Comment 3•8 years ago
|
||
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?
Flags: needinfo?(khuey)
I think I answered this question on IRC.
Flags: needinfo?(khuey)
Reporter | ||
Comment 5•8 years ago
|
||
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.
Reporter | ||
Comment 6•8 years ago
|
||
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.
Attachment #8734424 -
Attachment is obsolete: true
Attachment #8736377 -
Flags: review?(khuey)
Reporter | ||
Comment 7•8 years ago
|
||
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.
Attachment #8736378 -
Flags: review?(khuey)
Reporter | ||
Comment 8•8 years ago
|
||
I'll finish the patch to implement the final step once I've gotten some feedback on the first two.
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?
Attachment #8736377 -
Flags: review?(khuey) → review+
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?
Attachment #8736378 -
Flags: review?(khuey) → review-
Reporter | ||
Updated•8 years ago
|
Assignee: ejpbruel → nobody
Updated•7 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 11•6 years ago
|
||
@Brian: is this something we should take a look at atm? If not, we should probably change to P3
Honza
Flags: needinfo?(bhackett1024)
Comment 12•6 years ago
|
||
(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.
Flags: needinfo?(bhackett1024)
Comment 13•5 years ago
|
||
I'm going to work on this in bug 1527203.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•