Closed
Bug 1517896
Opened 5 years ago
Closed 5 years ago
PersistentRooted<GCHashSet> does not work in ShellContext
Categories
(Core :: JavaScript: GC, enhancement, P3)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla67
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
Details
Attachments
(1 file, 1 obsolete file)
2.56 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
arai attempted to use PersistentRooted<GCHashSet> and got a !Empty() assertion during shutdown.
Assignee | ||
Comment 1•5 years ago
|
||
The problem seems to be that we don't actually delete the main ShellContext. (We do delete worker ones). This is not the minimal necessary change, I don't think -- when tracing through, it seemed wrong to be deleting the runtime before the context, so I swapped the order, then fixed up everything that broke. If that causes too many issues, I can undo that part.
Attachment #9034520 -
Flags: review?(jcoppeard)
Comment 2•5 years ago
|
||
Comment on attachment 9034520 [details] [diff] [review] Fix shell context shutdown ordering Review of attachment 9034520 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +11102,5 @@ > DestructSharedObjectMailbox(); > > CancelOffThreadJobsForRuntime(cx); > > + JS_SetContextPrivate(cx, nullptr); Do we not need to do this in the worker case? ::: js/src/threading/ProtectedData.cpp @@ +55,2 @@ > JSContext* cx = TlsContext.get(); > + MOZ_ASSERT_IF(cx, CurrentThreadCanAccessRuntime(cx->runtime())); Does this allow the check to pass if this is called on an unrelated (non-SM) thread? That seems bad.
Assignee | ||
Comment 3•5 years ago
|
||
Clearing out the context's private data isn't really necessary anywhere, it just felt dangerous to be running a bunch of code during shutdown with a dangling pointer. But you're right, I should clear it consistently. Fixed. I switched to using AutoNoteSingleThreadedRegion to suppress the checks, and it appears to shut down successfully. Do you have an opinion on whether a cx or rt should be shut down first, btw? Because if I didn't change that, then this patch would be simpler. Maybe it's just because my head is still stuck in a multiple-cx world that it seems wrong to me to blow away the runtime first and have a period where there is a cx without an rt. An rt without a cx didn't seem as bad.
Attachment #9034825 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•5 years ago
|
Attachment #9034520 -
Attachment is obsolete: true
Attachment #9034520 -
Flags: review?(jcoppeard)
Comment 4•5 years ago
|
||
Comment on attachment 9034825 [details] [diff] [review] Fix shell context shutdown ordering Review of attachment 9034825 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I see. I agree, it feels like we should destroy the context before the runtime.
Attachment #9034825 -
Flags: review?(jcoppeard) → review+
Updated•5 years ago
|
Priority: -- → P3
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd74f1bbff3 Fix shell context shutdown ordering, r=jonco
Comment 6•5 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in
before you can comment on or make changes to this bug.
Description
•