Once bug 1211903 lands, WorkerDebuggerManager can be simplified into a non-thread-safe class that only lives on the main thread.

Comment on attachment 8670791 [details] [diff] [review] WorkerDebuggerManager I assume that without bug 1211903 this is moot.

Bug 1211903 is about to land, so how about some review love for this patch? :-)

Needinfo for the review request in comment 3.

Comment on attachment 8701017 [details] [diff] [review] patch Review of attachment 8701017 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerDebuggerManager.cpp @@ +36,5 @@ > Run() override > { > + WorkerDebuggerManager* manager = WorkerDebuggerManager::Get(); > + MOZ_ASSERT(manager); > + nit: extra whitespace @@ +60,5 @@ > Run() override > { > + WorkerDebuggerManager* manager = WorkerDebuggerManager::Get(); > + MOZ_ASSERT(manager); > + here too @@ +138,5 @@ > +{ > + AssertIsOnMainThread(); > + > + if (!gWorkerDebuggerManager) { > + // The observer service now owns us until shutdown. does this comment belong on a different line? we haven't even done anything yet. @@ +163,5 @@ > +NS_IMETHODIMP > +WorkerDebuggerManager::Observe(nsISupports* aSubject, const char* aTopic, > + const char16_t* aData) > +{ > + if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) { Perhaps this should be WORKERS_SHUTDOWN_TOPIC? That's effectively where it was before.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > Comment on attachment 8701017 [details] [diff] [review] > patch > > Review of attachment 8701017 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +138,5 @@ > > +{ > > + AssertIsOnMainThread(); > > + > > + if (!gWorkerDebuggerManager) { > > + // The observer service now owns us until shutdown. > > does this comment belong on a different line? we haven't even done anything > yet. > > @@ +163,5 @@ > > +NS_IMETHODIMP > > +WorkerDebuggerManager::Observe(nsISupports* aSubject, const char* aTopic, > > + const char16_t* aData) > > +{ > > + if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) { > > Perhaps this should be WORKERS_SHUTDOWN_TOPIC? That's effectively where it > was before. Both these things were copies almost verbatim from the RuntimeService because I wanted the lifetime of the WorkerDebuggerManager to match that of the former.

Try push for this patch, with comments by khuey addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa6b5c70e1be

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/68a72e3eae2d

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=21143697&repo=mozilla-inbound

Ugh, I forgot about that compile error. Sorry Carsten. I have a new patch prepared. Normally, I would push it to try first to make sure we don't have another backout, but seeing that try is closed right now, and this seems to be the only feature, I think we can risk pushing to inbound directly.