917909 - Remove dummy context from workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

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

We may still need it to deal with ctypes.  https://hg.mozilla.org/releases/mozilla-release/annotate/79c8eca7bdd1/dom/workers/RuntimeService.cpp#l875

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=3e4672769ce4

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop asserting that the last JSContext is destroyed outside of GC. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove mLastContext hackery in workers. v1

Comment 7

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

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/

Comment 8

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd8b3414e93
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f66b39396f38

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fbd8b3414e93
https://hg.mozilla.org/mozilla-central/rev/f66b39396f38