If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Intermittent webaudio/test/test_audioParamTimelineDestinationOffset.html,test_mediaDecoding.html | application crashed [@ js::CompartmentChecker::check] after Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY)

RESOLVED FIXED in Firefox 50

Status

()

Core
Web Audio
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jesup, Assigned: mccr8)

Tracking

({crash, intermittent-failure})

unspecified
mozilla51
crash, intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Intermittent I noticed on inbound which implies a GC issue

Assertion in CompartmentChecker::check --  GC-marked-gray object passed into JS_StealArrayBufferContents

https://treeherder.mozilla.org/logviewer.html#?job_id=34081663&repo=mozilla-inbound#L25064

JS_StealArrayBufferContents() was called from:

AudioBuffer::StealJSArrayDataIntoSharedChannels [AudioBuffer.cpp:391)
AudioBuffer::GetThreadSharedChannelsForRate [AudioBuffer.cpp:411)
AudioBufferSourceNode::SendBufferParameterToStream [AudioBufferSourceNode.cpp:703)
(Reporter)

Comment 1

a year ago
Terrence: what are the likely sources of this type of error/assertion, and what are the likely consequences in opt builds without the assertion?

I see bug 1291364 where bz did some work to avoid this in workers; are patterns like that what we should look to to avoid this?  (Note I haven't actually looked at the patch, just the bug.)
Rank: 23
Flags: needinfo?(terrence)
(Reporter)

Comment 2

a year ago
Looks like bug 1292852 is a dumping ground for assertions generated by this
See Also: → bug 1292852
Summary: Assertion due to a GC-marked-gray object passed into JS_StealArrayBufferContents → Intermittent dom/media/webaudio/test/test_audioParamTimelineDestinationOffset.html | application crashed [@ js::CompartmentChecker::check] after Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY)
(Reporter)

Comment 3

a year ago
Terrence has a proposed fix for this class of assertions
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1292852
Actually, this is probably a long-tail situation and the stack is pretty different.

The problem is that if we have a GC-black-to-CC-gray edge then there is a UAF bug somewhere in the system. This assertion ensures that we never pass a gray object into SpiderMonkey, which would imply that an ExposeToActiveJS was missed somewhere. There may be many sites that are missing this barrier.
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: DUPLICATE → ---
Going to need more room in the summary, then, even just for a webaudio dumping ground.
Summary: Intermittent dom/media/webaudio/test/test_audioParamTimelineDestinationOffset.html | application crashed [@ js::CompartmentChecker::check] after Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY) → Intermittent webaudio/test/test_audioParamTimelineDestinationOffset.html,test_mediaDecoding.html | application crashed [@ js::CompartmentChecker::check] after Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY)
Duplicate of this bug: 1296061
(Reporter)

Comment 7

a year ago
(In reply to Terrence Cole [:terrence] from comment #4)
> Actually, this is probably a long-tail situation and the stack is pretty
> different.
> 
> The problem is that if we have a GC-black-to-CC-gray edge then there is a
> UAF bug somewhere in the system. This assertion ensures that we never pass a
> gray object into SpiderMonkey, which would imply that an ExposeToActiveJS
> was missed somewhere. There may be many sites that are missing this barrier.

Is there a good explanation/example of the requirements around ExposeToActiveJS that would help us figure out what's being done wrong here?  GC-black-to-CC-edge means little to me and nothing to most of our devs (I can infer a little from it), and if we're going to have this assertion there to catch real problems, it would help to have something to point people who hit it in the right direction (for example, a comment pointing to a wiki page or bug with the info, or info in the comment itself.)
Flags: needinfo?(terrence)

Comment 8

a year ago
43 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 25
* autoland: 10
* mozilla-central: 3
* fx-team: 3
* try: 2

Platform breakdown:
* linux64: 22
* linux32: 12
* windows7-32-vm: 8
* windows7-32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-19&endday=2016-08-19&tree=all

Comment 9

a year ago
72 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 44
* autoland: 17
* fx-team: 5
* mozilla-central: 4
* try: 2

Platform breakdown:
* linux64: 30
* linux32: 28
* windows7-32-vm: 8
* windows7-32: 3
* android-4-3-armv7-api15: 2
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-15&endday=2016-08-21&tree=all
39 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 20
* autoland: 15
* mozilla-central: 2
* try: 1
* fx-team: 1

Platform breakdown:
* linux64: 18
* linux32: 13
* android-4-3-armv7-api15: 3
* windows7-32-vm: 2
* osx-10-10: 2
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-22&endday=2016-08-22&tree=all
> Is there a good explanation/example of the requirements around ExposeToActiveJS
> that would help us figure out what's being done wrong here?

The fundamental requirement is this: If you have a JS thing that you cycle collect (in this case the things in AudioBuffer.mJSChannels) then you must ensure that ExposeToActiveJS is called any time you're relying on the GC keeping it alive.

This requirement is fairly complicated to test for, because it depends on lifetime analysis of the cycle-collected member.  So in practice we assert a simpler invariant: any time you're passing a thing to JSAPI functions that do anything interesting it needs to have been exposed to active JS.  In theory, we should really assert if a non-exposed thing is stored in a Rooted, but in practice this would have too many false positives right now (e.g. places where the store into Rooted happens right before an Expose call).
(Reporter)

Comment 12

a year ago
karl - is this something you could take, or at least scope the work required to deal with it?  Thanks!
Rank: 23 → 15
Flags: needinfo?(karlt)
Priority: P2 → P1
(Assignee)

Comment 13

a year ago
The basic fix is to call JS::ExposeObjectToActiveJS whenever you read a value out of channelArray, except for the cycle collector helper functions. The nicest way to deal with that would be a subclass of JS::Heap that does that automatically, with a color-preserving getter for the CC to use, and then change mJSChannels to use it. Terrence, what do you think about that?
(Assignee)

Comment 14

a year ago
> you read a value out of channelArray
Sorry, I meant mJSChannels here.
43 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 14
* mozilla-inbound: 12
* fx-team: 8
* mozilla-central: 7
* try: 2

Platform breakdown:
* linux32: 17
* linux64: 12
* windows7-32-vm: 4
* windows7-32: 3
* osx-10-10: 3
* windowsxp: 2
* windows8-64: 1
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-23&endday=2016-08-23&tree=all
(In reply to Andrew McCreight [:mccr8] from comment #14)
Terrence is working on this in bug 1297558.
(Assignee)

Updated

a year ago
Depends on: 1297558
43 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 18
* mozilla-inbound: 13
* fx-team: 5
* mozilla-central: 4
* try: 3

Platform breakdown:
* linux64: 16
* linux32: 14
* windowsxp: 4
* windows8-64: 3
* windows7-32-vm: 3
* osx-10-10: 2
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-24&endday=2016-08-24&tree=all
51 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 30
* mozilla-inbound: 16
* try: 3
* fx-team: 2

Platform breakdown:
* linux64: 30
* linux32: 14
* windows8-64: 3
* windows7-32-vm: 2
* osx-10-10: 1
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-25&endday=2016-08-25&tree=all
71 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 33
* autoland: 25
* fx-team: 6
* mozilla-central: 4
* try: 3

Platform breakdown:
* linux64: 40
* linux32: 21
* windows7-32-vm: 3
* osx-10-10: 3
* windows8-64: 2
* windows7-32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-26&endday=2016-08-26&tree=all
271 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* autoland: 106
* mozilla-inbound: 101
* fx-team: 26
* mozilla-central: 19
* try: 18
* oak: 1

Platform breakdown:
* linux64: 124
* linux32: 86
* windows8-64: 15
* windows7-32-vm: 15
* osx-10-10: 11
* windowsxp: 8
* android-4-3-armv7-api15: 8
* windows7-32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-22&endday=2016-08-28&tree=all
(Assignee)

Comment 21

a year ago
Terrence said his big fix will take a bit so I'll do something in the interim to stop the bleeding.
Assignee: nobody → continuation
Flags: needinfo?(terrence)
Flags: needinfo?(karlt)
(Assignee)

Comment 22

a year ago
I looked at 8 or so crashes and they were all in JS_StealArrayBufferContents() so I'll just patch that up.
Keywords: leave-open
Comment hidden (mozreview-request)
53 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 26
* mozilla-inbound: 19
* mozilla-central: 4
* try: 3
* fx-team: 1

Platform breakdown:
* linux64: 29
* linux32: 21
* android-4-3-armv7-api15: 2
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-29&endday=2016-08-29&tree=all
(Assignee)

Comment 25

a year ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1581c2cd3112
(Assignee)

Updated

a year ago
Attachment #8786159 - Flags: review?(karlt) → review?(terrence)
(Assignee)

Comment 26

a year ago
Well, maybe it makes more sense for Terrence to review this.

Comment 27

a year ago
mozreview-review
Comment on attachment 8786159 [details]
Bug 1296275 - Be better about exposing to active js in AudioBuffer::StealJSArrayDataIntoSharedChannels().

https://reviewboard.mozilla.org/r/75124/#review73196

Looks good to me.

Comment 28

a year ago
mozreview-review
Comment on attachment 8786159 [details]
Bug 1296275 - Be better about exposing to active js in AudioBuffer::StealJSArrayDataIntoSharedChannels().

https://reviewboard.mozilla.org/r/75124/#review73198
Attachment #8786159 - Flags: review?(terrence) → review+

Comment 29

a year ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a8e5d79f09c
Be better about exposing to active js in AudioBuffer::StealJSArrayDataIntoSharedChannels(). r=terrence
54 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 21
* autoland: 14
* try: 7
* mozilla-central: 6
* fx-team: 6

Platform breakdown:
* linux64: 35
* linux32: 17
* windowsxp: 1
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-30&endday=2016-08-30&tree=all

Comment 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a8e5d79f09c
24 automation job failures were associated with this bug yesterday.

Repository breakdown:
* try: 12
* mozilla-inbound: 9
* fx-team: 2
* mozilla-central: 1

Platform breakdown:
* linux64: 11
* linux32: 10
* windows7-32-vm: 2
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-31&endday=2016-08-31&tree=all
(Assignee)

Comment 33

a year ago
I'll check this in a day or two to see if there are any residual crashes once the bulk of them have been fixed (hopefully) by my patch.
Flags: needinfo?(continuation)
142 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 49
* autoland: 41
* try: 30
* mozilla-central: 11
* fx-team: 10
* ash: 1

Platform breakdown:
* linux64: 78
* linux32: 53
* windows7-32-vm: 3
* windowsxp: 2
* windows8-64: 2
* osx-10-10: 2
* android-4-3-armv7-api15: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1296275&startday=2016-08-29&endday=2016-09-04&tree=all
(Assignee)

Updated

a year ago
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Flags: needinfo?(continuation)
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Keywords: leave-open
Comment on attachment 8786159 [details]
Bug 1296275 - Be better about exposing to active js in AudioBuffer::StealJSArrayDataIntoSharedChannels().

Approval Request Comment
[Feature/regressing bug #]: Incremental CC, found by the assertions in bug 1283634.
[User impact if declined]: Intermittent crashes.
[Describe test coverage new/current, TreeHerder]: It's been on TH for a week and no longer hits the assertions.
[Risks and why]: There was a slow trickle of hard-to-exploit UAF crashes in previous branches caused by missing ExposeToActiveJS barriers in a few places. We added an assertion that catches these in bug 1283634. We'd like to uplift the fixes to Aurora to solve the crashes 6 weeks earlier than we otherwise might. The impact is relatively low, but the patches are also extremely simple and low risk. Aurora seems like the right balance here.
[String/UUID change made/needed]: None.
Attachment #8786159 - Flags: approval-mozilla-aurora?
status-firefox51: --- → fixed
Target Milestone: --- → mozilla51

Updated

a year ago
status-firefox50: --- → affected
Comment on attachment 8786159 [details]
Bug 1296275 - Be better about exposing to active js in AudioBuffer::StealJSArrayDataIntoSharedChannels().

Crash fix, has stabilized on Nightly for ~10 days, Aurora50+
Attachment #8786159 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 37

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0a1f64d2110e
status-firefox50: affected → fixed
Flags: in-testsuite+
See Also: bug 1292852
You need to log in before you can comment on or make changes to this bug.