Closed
Bug 1027221
Opened 10 years ago
Closed 10 years ago
JS_WrapValue on a permanent atom on a DOM worker thread fatally asserts if the main thread is in an incremental GC
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: bzbarsky, Assigned: terrence)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files)
2.52 KB,
patch
|
terrence
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
billm
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This is similar to bug 1024170 and blocks fixing bug 965644.
What happens is that JS_WrapValue calls ExposeValueToActiveJS, which calls ExposeGCThingToActiveJS, which checks whether IsIncrementalBarrierNeededOnGCThing and if so does IncrementalReferenceBarrier.
IncrementalReferenceBarrier in turn does:
972 Zone *zone = kind == JSTRACE_OBJECT
973 ? static_cast<JSObject *>(cell)->zone()
974 : cell->tenuredZone();
which lands us in:
js::gc::Cell::tenuredZone (this=0x114f00b68) at Heap.h:1062
1062 JS_ASSERT(CurrentThreadCanAccessZone(zone));
and that assertion fails, because the current thread is the DOM worker thread but the zone is the main-thread atom zone, for a permanent atom.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
We need to track this for branches if this is an actual threading problem, not just an assert...
Blocks: 966452
Reporter | ||
Comment 4•10 years ago
|
||
Terrence, you said you had a fix for this in some other patch? Is that going on 31, or do we need a targeted fix here?
Flags: needinfo?(terrence)
Assignee | ||
Comment 5•10 years ago
|
||
It will be in 31 eventually, but we should still do an uplift of the smaller 2-line patch in the meantime.
Flags: needinfo?(terrence)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8446780 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0216995f0aa8
https://hg.mozilla.org/mozilla-central/rev/680aabc0973c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 9•10 years ago
|
||
Terrence, could you fill the uplift request? (if possible today, Monday, to get the patch in beta 6).
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → fixed
Flags: needinfo?(terrence)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8446780 [details] [diff] [review]
allow_igc_barriers_on_off_thread_permanent_atoms-v0.diff
(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Terrence, could you fill the uplift request? (if possible today, Monday, to
> get the patch in beta 6).
Thanks for the reminder!
Approval Request Comment
[Feature/regressing bug #]: PermanentAtoms or Promises on WebWorkers, whichever came later.
[User impact if declined]: Intermittent crashes if WebWorkers use Promises.
[Describe test coverage new/current, TBPL]: Complete test coverage.
[Risks and why]: Extremely low.
[String/UUID change made/needed]: None.
Attachment #8446780 -
Flags: approval-mozilla-beta?
Attachment #8446780 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(terrence)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8442249 [details] [diff] [review]
testcase
Approval Request Comment
Test code covering the fix.
Attachment #8442249 -
Flags: review+
Attachment #8442249 -
Flags: approval-mozilla-beta?
Attachment #8442249 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8446780 -
Flags: approval-mozilla-beta?
Attachment #8446780 -
Flags: approval-mozilla-beta+
Attachment #8446780 -
Flags: approval-mozilla-aurora?
Attachment #8446780 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8442249 -
Flags: approval-mozilla-beta?
Attachment #8442249 -
Flags: approval-mozilla-beta+
Attachment #8442249 -
Flags: approval-mozilla-aurora?
Attachment #8442249 -
Flags: approval-mozilla-aurora+
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Terrence, can you please look into the failures in comment 13?
Flags: needinfo?(terrence)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Terrence, can you please look into the failures in comment 13?
I suspected this to be caused by StringIsPermanentAtom not being threadsafe, but that is not the case. It appears to be the new test itself falling over when wrapping the promise result.
Boris, is the test expected to work as is on aurora and beta?
Flags: needinfo?(terrence) → needinfo?(bzbarsky)
Reporter | ||
Comment 16•10 years ago
|
||
The test should work, modulo JS engine bugs.
Are you running into bug 1024170, perhaps? I have no idea why that did not get backported; it should be.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #16)
> The test should work, modulo JS engine bugs.
>
> Are you running into bug 1024170, perhaps? I have no idea why that did not
> get backported; it should be.
Gah, good catch! I assumed that had been backported already.
Comment 18•10 years ago
|
||
I approved the patch from bug 1024170
Comment 19•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6ae6dbf114
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb5036debda7
https://hg.mozilla.org/releases/mozilla-beta/rev/629ca8a4ecdd
https://hg.mozilla.org/releases/mozilla-beta/rev/60bda4a9bf60
Is this something that impacts B2G v1.4 (b2g30) and should be considered for blocking status?
Reporter | ||
Comment 20•10 years ago
|
||
It impacts that branch, I believe (unless IGC is off on b2g, which I doubt). Whether it should be blocking, I'm not sure. It's not clear to me whether this is a problem in opt builds.
Comment 21•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6ae6dbf114
> https://hg.mozilla.org/releases/mozilla-aurora/rev/bb5036debda7
>
> https://hg.mozilla.org/releases/mozilla-beta/rev/629ca8a4ecdd
> https://hg.mozilla.org/releases/mozilla-beta/rev/60bda4a9bf60
>
> Is this something that impacts B2G v1.4 (b2g30) and should be considered for
> blocking status?
Yes, it impacts. It should not be blocking since these MOZ_ASSERT assertions do not cause any matter in opt builds.
Flags: needinfo?(terrence)
Updated•10 years ago
|
status-b2g-v1.4:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•