1078354 - Crash in webaudio while stability testing

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Greg Grisco]

[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 ]

Comment 1

[Greg Grisco]

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.

Comment 2

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

[Blocking Requested - why for this release]:

Comment 3

[Greg Grisco]

These were seen with

gaia: a00d102abfe8ae15c4fd14771efa2335c6d3b8d9
gecko:19e14f4003cafc106784a4984018ab28e3d9c1ff

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: stack trace

Comment 5

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: .extra file logs

Comment 6

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - engr_build

Comment 7

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - engr_build

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Probably a dupe of bug 1068981.  We're going to uplift and wait for CAF to confirm.

Comment 9

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 10

[bhavana bajaj [:bajaj]]

BLocking preemptively as its a crash, once confirmed will DUP it.

Comment 13

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_2.0.01.04.00.114.103
Moz BuildID: 20140924000243
B2G Version: 2.1
Gecko Version: 34.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=93a99bea0b40d81bd063f7d8b1964dc1ba35ba7b
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=c896bc5d04b8c08dcddd78195901503a3578a08f
Patches: bug 1055299, bug 1074703, bug 1065511, bug 1063066, bug 1068978, bug 1072021, bug 1073088, bug 1073419, bug 1067205, bug 1076327, bug 1070431, bug 1068394, bug 1043712, bug 1059573, bug 1068981, bug 1051636, bug 1051633, bug 1068571, bug 1063568, bug 1074419

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 15

[Maire Reavy [:mreavy]]

Paul -- Can you take this?

Comment 16

[Greg Grisco]

(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.

Comment 17

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(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.

Comment 18

[Paul Adenot (:padenot)]

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

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Can you confirm if this is still a bug or not on versions with the bug 1068981 patch?  Thanks.

Comment 20

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(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.

Comment 21

[Paul Adenot (:padenot)]

Attachment: Part 1 - Make sure we are not waking up an OfflineGraphDriver. r=

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.

Comment 22

[Paul Adenot (:padenot)]

Attachment: Part 2 - Don't try to measure a PeriodicWave size when an OscillatorNode is using a basic waveform. r=

OscillatorNode don't always have mPeriodicWave set, so we need to null-check
this.

Comment 23

[Eric Rahm [: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.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

If we're ready to land, please request b2g34 approval ASAP

Comment 25

[Paul Adenot (: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

Comment 26

[Paul Adenot (:padenot)]

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

Comment 27

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9601061181bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6146f9dfb0

Comment 28

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(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.

Comment 29

[bhavana bajaj [:bajaj]]

(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?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9601061181bd
https://hg.mozilla.org/mozilla-central/rev/ad6146f9dfb0

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Does this affect Aurora35/Beta34?

Comment 32

[Paul Adenot (:padenot)]

(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.

Comment 33

[Paul Adenot (: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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(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 35

[Paul Adenot (:padenot)]

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

Comment 36

[Paul Adenot (:padenot)]

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

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bb5349b9b38
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d6147b9c59e

https://hg.mozilla.org/releases/mozilla-beta/rev/9d0a16097623
https://hg.mozilla.org/releases/mozilla-beta/rev/b185e7a13e18

Comment 39

[Yeojin Chung [:YeojinC]]

Unable to verify at this location as this issue involves an automated test.