Closed Bug 917909 Opened 6 years ago Closed 6 years ago

Remove dummy context from workers

Categories

(Core :: DOM: Workers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

Workers keeps around a dummy context to appease the garbage collector, which used to do a rambo GC when the last context was destroyed. I fixed that in bug 905926, so now that context can go away.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> We may still need it to deal with ctypes. 
> https://hg.mozilla.org/releases/mozilla-release/annotate/79c8eca7bdd1/dom/
> workers/RuntimeService.cpp#l875

I'm removing the ctypes context in the bug that depends on this one.

Regardless though, I'd hoped this could land as an unencumbered dep. I'm not sure exactly why we need to assert that we don't destroy our last context during a GC. Bill, is that assertion still relevant post bug 905926?
Flags: needinfo?(wmccloskey)
The assertion was added by peterv in bug 475737 at Igor's request. It's never made clear in that bug why we needed it. JSContexts used to be a lot more important during GC than they are now, so it seems likely that the restriction is outdated. I think we should just remove it.
Flags: needinfo?(wmccloskey)
Attachment #806925 - Flags: review?(wmccloskey)
Attachment #806925 - Flags: review?(khuey)
Comment on attachment 806925 [details] [diff] [review]
Part 2 - Remove mLastContext hackery in workers. v1

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

Assuming you've addressed everything I warned you about, r=me.

::: dom/workers/RuntimeService.cpp
@@ +840,5 @@
>      auto rtPrivate = static_cast<WorkerThreadRuntimePrivate*>(JS_GetRuntimePrivate(Runtime()));
>      delete rtPrivate;
>      JS_SetRuntimePrivate(Runtime(), nullptr);
>  
> +    // The  worker global should be unrooted and the shutdown cycle collection

nit: only one space between 'The' and 'worker'

@@ +847,4 @@
>      // in cycles that were broken during CC shutdown.
>      nsCycleCollector_shutdown();
>  
> +    // The CC is shut down, and this will GC, so make sure we don't try to CC

s/this/the superclass destructor/
Attachment #806925 - Flags: review?(khuey) → review+
Attachment #806924 - Flags: review?(wmccloskey) → review+
Attachment #806925 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/fbd8b3414e93
https://hg.mozilla.org/mozilla-central/rev/f66b39396f38
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.