Closed Bug 1212333 Opened 5 years ago Closed 5 years ago

WorkerDebuggerManager should live on the main thread.

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 1 obsolete file)

Once bug 1211903 lands, WorkerDebuggerManager can be simplified into a non-thread-safe class that only lives on the main thread.
Attached patch WorkerDebuggerManager (obsolete) — Splinter Review
Attachment #8670791 - Flags: review?(khuey)
Blocks: 1212344
Comment on attachment 8670791 [details] [diff] [review]
WorkerDebuggerManager

I assume that without bug 1211903 this is moot.
Attachment #8670791 - Flags: review?(khuey)
Blocks: 1214248
Attached patch patchSplinter Review
Bug 1211903 is about to land, so how about some review love for this patch? :-)
Attachment #8670791 - Attachment is obsolete: true
Attachment #8701017 - Flags: review?(khuey)
Needinfo for the review request in comment 3.
Flags: needinfo?(khuey)
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.
Attachment #8701017 - Flags: review?(khuey) → review+
(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
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.
Flags: needinfo?(ejpbruel)
https://hg.mozilla.org/mozilla-central/rev/86f89d86b235
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.