1212333 - WorkerDebuggerManager should live on the main thread.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Eddy Bruel [:ejpbruel]]

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

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: WorkerDebuggerManager

Comment 2

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

Comment on attachment 8670791 [details] [diff] [review]
WorkerDebuggerManager

I assume that without bug 1211903 this is moot.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: patch

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

Comment 4

[Eddy Bruel [:ejpbruel]]

Needinfo for the review request in comment 3.

Comment 5

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

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.

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 7

[Eddy Bruel [:ejpbruel]]

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

Comment 8

[Pulsebot]

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

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Eddy Bruel [:ejpbruel]]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86f89d86b235

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86f89d86b235