Crash in webaudio while stability testing

RESOLVED FIXED in Firefox 34

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ggrisco, Assigned: padenot)

Tracking

({crash})

unspecified
mozilla36
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 wontfix, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [caf-crash 368][caf priority: p2][CR 729453][b2g-crash], crash signature)

Attachments

(6 attachments)

Reporter

Description

5 years ago
[Blocking Requested - why for this release]:

We've seen a crash with the following signature more than 50 times in latest build:

[@ mozilla::dom::PeriodicWave::SizeOfExcludingThisIfNotShared(unsigned int (*)(void const*)) const | mozilla::dom::PeriodicWave::SizeOfIncludingThisIfNotShared(unsigned int (*)(void const*)) const | mozilla::dom::OscillatorNode::SizeOfExcludingThis(unsigned int (*)(void const*)) const | mozilla::BaseMediaResource::SizeOfIncludingThis(unsigned int (*)(void const*)) const ]
Reporter

Comment 1

5 years ago
Just realized this was on an engineering build.  Will withdraw this for now, but I think we will see this on official builds, so it may get re-opened.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
blocking-b2g: 2.0? → ---
[Blocking Requested - why for this release]:
Status: RESOLVED → REOPENED
blocking-b2g: --- → 2.1?
Resolution: INVALID → ---
Reporter

Comment 3

5 years ago
These were seen with

gaia: a00d102abfe8ae15c4fd14771efa2335c6d3b8d9
gecko:19e14f4003cafc106784a4984018ab28e3d9c1ff
Probably a dupe of bug 1068981.  We're going to uplift and wait for CAF to confirm.
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Probably a dupe of bug 1068981.  We're going to uplift and wait for CAF to
> confirm.

We asked our test team to confirm and we will update shortly.
Whiteboard: [b2g-crash] → [CR 735848][b2g-crash]
Whiteboard: [CR 735848][b2g-crash] → [caf priority: p2][CR 735848][b2g-crash]
BLocking preemptively as its a crash, once confirmed will DUP it.
blocking-b2g: 2.1? → 2.1+
Duplicate of this bug: 1079461
Duplicate of this bug: 1079467
Whiteboard: [caf priority: p2][CR 735848][b2g-crash] → [caf-crash 368][caf priority: p2][CR 735848][b2g-crash]
Keywords: crash
Whiteboard: [caf-crash 368][caf priority: p2][CR 735848][b2g-crash] → [caf-crash 368][caf priority: p2][CR 729453][b2g-crash]
My best guess is that it's in ThreadSharedFloatArrayBufferList::SizeOfExcludingThis(), in particular looking a mContents, which is an AutoFallibleTArray<Storage,2> -- and we're crashing in nsAutoPtr somewhere with a null ptr.  ThreadSharedFloatArrayBufferList::SizeOfExcludingThis() doesn't null-check mContents[i], and the ThreadSharedFloatArrayBufferList(aCount) constructor explicitly constructs them with null data (mContents.SetLength()).

This might not be the problem, but it's certainly fishy/risky.

It's likely not bug 1068981 (I *think* the report from Greg indicates the uplift of that bug didn't fix it; I can't find the gecko rev specified above in comment 3; I suspect it's a git rev not an hg rev).

Is there a regression range?
Flags: needinfo?(padenot)
Flags: needinfo?(ggrisco)
Paul -- Can you take this?
Assignee: nobody → padenot
Reporter

Comment 16

5 years ago
(In reply to Randell Jesup [:jesup] from comment #14)

> It's likely not bug 1068981 (I *think* the report from Greg indicates the
> uplift of that bug didn't fix it; 

Right.  Comment #13 basically says that the crash happened on a build that had the patch from bug 1068981 applied.
Flags: needinfo?(ggrisco)
(In reply to Greg Grisco from comment #16)
> (In reply to Randell Jesup [:jesup] from comment #14)
> 
> > It's likely not bug 1068981 (I *think* the report from Greg indicates the
> > uplift of that bug didn't fix it; 
> 
> Right.  Comment #13 basically says that the crash happened on a build that
> had the patch from bug 1068981 applied.

I think that Comment #13 is wrong here.  We are still waiting to see this crash is reproduced in any build which has Gonk Version >= AU_LINUX_GECKO_B2G_KK_2.0.01.04.00.114.104

Sorry for confusions.
If this is not fixed, here is a possible explanation:

PeriodicWaves are not a widely used feature of Web Audio API. They are not used in Gaia, as far as I know, and we should not reach this code at all in the first place (or maybe reporters are going on websites that use PeriodicWave? I don't know). I found this [0], where a nullcheck on mPeriodicWave is missing. mozilla-central null checks this pointer, as it should be.

What version are we reproducing on, here? 2.0 has the bug per [0], but I'm not sure it's what we are looking at.

[0]: http://mxr.mozilla.org/mozilla-b2g32_v2_0/source/content/media/webaudio/OscillatorNode.cpp#556
Flags: needinfo?(padenot)
Can you confirm if this is still a bug or not on versions with the bug 1068981 patch?  Thanks.
Flags: needinfo?(tkundu)
Flags: needinfo?(ggrisco)
(In reply to Randell Jesup [:jesup] from comment #19)
> Can you confirm if this is still a bug or not on versions with the bug
> 1068981 patch?  Thanks.

Sorry we are still waiting input from our test team. We will confirm you asap. Sorry for unexpected delays.
Flags: needinfo?(ggrisco)

Updated

5 years ago
No longer blocks: CAF-v2.1-FC-metabug

Updated

5 years ago
I still want to assert in the method instead of making it a no-op because I'm
going to work on AudioContext sleep and wakeup soon, and this will catch errors.
Attachment #8507974 - Flags: review?(rjesup)
Attachment #8507974 - Flags: review?(rjesup) → review+
OscillatorNode don't always have mPeriodicWave set, so we need to null-check
this.
Attachment #8507977 - Flags: review?(erahm)
Comment on attachment 8507977 [details] [diff] [review]
Part 2 - Don't try to measure a PeriodicWave size when an OscillatorNode is using a basic waveform. r=

Review of attachment 8507977 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch! r=me

As a followup could you file a bug to actually implement unit tests for web audio memory reporting? We've been burned by this too many times.
Attachment #8507977 - Flags: review?(erahm) → review+
If we're ready to land, please request b2g34 approval ASAP
Flags: needinfo?(padenot)
Comment on attachment 8507974 [details] [diff] [review]
Part 1 - Make sure we are not waking up an OfflineGraphDriver. r=

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 848954
User impact if declined: possible crash when requesting memory usage in about:memory and using an OfflineAudioContext
Testing completed: locally (this was 100% reproducible before)
Risk to taking this patch (and alternatives if risky): low, this is triggered by dmd/about:memory
String or UUID changes made by this patch: none
Flags: needinfo?(padenot)
Attachment #8507974 - Flags: approval-mozilla-b2g34?
Comment on attachment 8507977 [details] [diff] [review]
Part 2 - Don't try to measure a PeriodicWave size when an OscillatorNode is using a basic waveform. r=

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 967817
User impact if declined: possible crash when requesting memory usage in about:memory and using an OscillatorNode using a basic waveform (a rather common scenario)
Testing completed: locally (this was 100% reproducible before)
Risk to taking this patch (and alternatives if risky): low, this is triggered by dmd/about:memory
String or UUID changes made by this patch: none
(In reply to Randell Jesup [:jesup] from comment #19)
> Can you confirm if this is still a bug or not on versions with the bug
> 1068981 patch?  Thanks.

We are not seeing this issue anymore in v2.1 after landing fix from bug 1068981. Sorry for delayed confirmation.
Flags: needinfo?(tkundu) → needinfo?(padenot)
(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #28)
> (In reply to Randell Jesup [:jesup] from comment #19)
> > Can you confirm if this is still a bug or not on versions with the bug
> > 1068981 patch?  Thanks.
> 
> We are not seeing this issue anymore in v2.1 after landing fix from bug
> 1068981. Sorry for delayed confirmation.

:jesup/Paul,

If the patch in 1068981 fixes the issue, do we still need this uplift?
Flags: needinfo?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/9601061181bd
https://hg.mozilla.org/mozilla-central/rev/ad6146f9dfb0
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Does this affect Aurora35/Beta34?
(In reply to bhavana bajaj [:bajaj] from comment #29)
> (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from
> comment #28)
> > (In reply to Randell Jesup [:jesup] from comment #19)
> > > Can you confirm if this is still a bug or not on versions with the bug
> > > 1068981 patch?  Thanks.
> > 
> > We are not seeing this issue anymore in v2.1 after landing fix from bug
> > 1068981. Sorry for delayed confirmation.
> 
> :jesup/Paul,
> 
> If the patch in 1068981 fixes the issue, do we still need this uplift?

We do, this fixes two other crashes with somewhat similar STR.
Flags: needinfo?(padenot)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31)
> Does this affect Aurora35/Beta34?

It does, yes. Those patches are not risky (they only touch code used by about:memory), we can uplift them if needed.
Attachment #8507974 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
(In reply to Paul Adenot (:padenot) from comment #33)
> It does, yes. Those patches are not risky (they only touch code used by
> about:memory), we can uplift them if needed.

Please request Aurora/Beta approval in that case :)

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1e052a34c4a0
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f05da96a08bc
Comment on attachment 8507974 [details] [diff] [review]
Part 1 - Make sure we are not waking up an OfflineGraphDriver. r=

See comment 25 and comment 26
Attachment #8507974 - Flags: approval-mozilla-beta?
Attachment #8507974 - Flags: approval-mozilla-aurora?
Comment on attachment 8507977 [details] [diff] [review]
Part 2 - Don't try to measure a PeriodicWave size when an OscillatorNode is using a basic waveform. r=

See comment 25 and comment 26
Attachment #8507977 - Flags: approval-mozilla-beta?
Attachment #8507977 - Flags: approval-mozilla-aurora?
Comment on attachment 8507974 [details] [diff] [review]
Part 1 - Make sure we are not waking up an OfflineGraphDriver. r=

As this is confined to about:memory, there doesn't seem to be any risk for a normal browsing scenario. 
Beta+
Aurora+
Attachment #8507974 - Flags: approval-mozilla-beta?
Attachment #8507974 - Flags: approval-mozilla-beta+
Attachment #8507974 - Flags: approval-mozilla-aurora?
Attachment #8507974 - Flags: approval-mozilla-aurora+
Attachment #8507977 - Flags: approval-mozilla-beta?
Attachment #8507977 - Flags: approval-mozilla-beta+
Attachment #8507977 - Flags: approval-mozilla-aurora?
Attachment #8507977 - Flags: approval-mozilla-aurora+
Unable to verify at this location as this issue involves an automated test.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.