Remove dummy context from workers

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla27
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 2

5 years ago
(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)
(Assignee)

Comment 5

5 years ago
Created attachment 806924 [details] [diff] [review]
Part 1 - Stop asserting that the last JSContext is destroyed outside of GC. v1
Attachment #806924 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

5 years ago
Created attachment 806925 [details] [diff] [review]
Part 2 - Remove mLastContext hackery in workers. v1
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.