Bug 77283 – Layout Test webaudio/panner-set-model.html crashes on debug Chromium bots

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Levi Weintraub]

webaudio/panner-set-model.html crashes on debug builds on all three Chromium platforms.

Win Stack:
SHOULD NEVER BE REACHED
Backtrace:
	std::_Init_locks::operator= [0x03345834+1439892]
	std::_Init_locks::operator= [0x032F413F+1106335]
	WebKit::WebData::data [0x031847D7+24863416]
	v8::Locker::StopPreemption [0x013AA254+726532]
	v8::Locker::StopPreemption [0x013AE6BE+744046]
	v8::Locker::StopPreemption [0x013AC784+736052]
	v8::Locker::StopPreemption [0x013A9F3E+725742]
	v8::Locker::StopPreemption [0x015E125B+3048971]
	v8::Locker::StopPreemption [0x015E4964+3063060]
	(No symbol) [0x0C308736]
	(No symbol) [0x0C3EC717]
	(No symbol) [0x0C3EBEF0]
	(No symbol) [0x0C31F2B9]
	(No symbol) [0x0C30B0CA]
	v8::Locker::StopPreemption [0x013517B9+363369]
	v8::Locker::StopPreemption [0x01351544+362740]
	v8::Script::Run [0x012CF57B+587]
	WebKit::WebData::data [0x0217C594+8052853]
	WebKit::WebData::data [0x0217C050+8051505]
	WebKit::WebData::data [0x021A66E5+8225222]
	WebKit::WebData::data [0x02B8E6AD+18612622]
	WebKit::WebData::data [0x02B8E0F2+18611155]
	WebKit::WebData::data [0x02A795F3+17477844]
	WebKit::WebData::data [0x02A78C42+17475363]
	WebKit::WebData::data [0x02A05F5E+17005119]
	WebKit::WebData::data [0x02A06072+17005395]
	WebKit::WebData::data [0x02A0632E+17006095]
	WebKit::WebData::data [0x02A05C6F+17004368]
	WebKit::WebData::data [0x02A06D42+17008675]
	WebKit::WebData::data [0x02B9A283+18660708]
	WebKit::WebData::data [0x0227F331+9113106]
	WebKit::WebData::data [0x0226A80A+9028331]
	WebKit::WebData::data [0x01B21052+1387315]
	WebKit::WebData::data [0x01A6FED4+661941]
	WebKit::WebData::data [0x0226A68D+9027950]
	WebKit::WebData::data [0x0226A926+9028615]
	WebKit::WebData::data [0x025C898A+12559467]
	WebKit::WebData::data [0x025C0FDF+12528320]
	WebKit::WebData::data [0x025C9E43+12564772]
	WebKit::WebData::data [0x025C1A8E+12531055]
	WebKit::WebData::data [0x01B37BB3+1480340]
	webkit::ppapi::PluginInstance::ScrollRect [0x07F06F7F+882021]
	(No symbol) [0x004F8DA4]
	(No symbol) [0x00500BF0]
	(No symbol) [0x00500887]
	(No symbol) [0x0050030F]
	file_util::GetTempDir [0x00C72A2F+350259]
	file_util::GetTempDir [0x00C79FA5+380329]
	file_util::GetTempDir [0x00C7A233+380983]
	file_util::GetTempDir [0x00C7B0D6+384730]
	file_util::GetTempDir [0x00C99ED4+511192]
	file_util::GetTempDir [0x00C99502+508678]
	file_util::GetTempDir [0x00C750CC+360144]
	file_util::GetTempDir [0x00C79B8A+379278]
	file_util::GetTempDir [0x00C798DE+378594]
	file_util::GetTempDir [0x00C78BC0+375236]
	(No symbol) [0x004D9BFD]
	(No symbol) [0x00441079]
	(No symbol) [0x00483C67]
	(No symbol) [0x00452945]
	(No symbol) [0x00451927]
	(No symbol) [0x004F33F8]
LEAK: 2394 WebCoreNode
LEAK: 276 CachedResource
LEAK: 18 Page
LEAK: 2 XMLHttpRequest
LEAK: 18 Frame
LEAK: 122 RenderObject

Linux Stack:
SHOULD NEVER BE REACHED
third_party/WebKit/Source/WebCore/platform/audio/Panner.cpp(57) : static WTF::PassOwnPtr<WebCore::Panner> WebCore::Panner::create(unsigned int, float)
1   0x244c880
2   0x16003a2
3   0x1812695
4   0x9db21c
5   0x9deb92
6   0x9dd15d
7   0x9daf28
8   0xc03ac4
9   0xc06346
10  0x253af360420e
[29640:29640:13295607829157:ERROR:process_util_posix.cc(142)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x728226]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x6e2981]
	0x7f2852fa3af0
	WebCore::Panner::create() [0x244c88a]
	WebCore::AudioPannerNode::setPanningModel() [0x16003a2]
	WebCore::AudioPannerNodeInternal::panningModelAttrSetter() [0x1812695]
	v8::internal::JSObject::SetPropertyWithCallback() [0x9db21c]
	v8::internal::JSObject::SetPropertyForResult() [0x9deb92]
	v8::internal::JSReceiver::SetProperty() [0x9dd15d]
	v8::internal::JSReceiver::SetProperty() [0x9daf28]
	v8::internal::StoreIC::Store() [0xc03ac4]
	v8::internal::StoreIC_Miss() [0xc06346]
	0x253af360420e

Mac 10.6 Stack:
SHOULD NEVER BE REACHED
/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/audio/Panner.cpp(57) : static PassOwnPtr<WebCore::Panner> WebCore::Panner::create(PanningModel, float)
1   0x4005b94b WebCore::Panner::create(unsigned int, float)
2   0x40d1b241 WebCore::AudioPannerNode::setPanningModel(unsigned short)
3   0x410ecc2c WebCore::AudioPannerNodeInternal::panningModelAttrSetter(v8::Local<v8::String>, v8::Local<v8::Value>, v8::AccessorInfo const&)
4   0x3f0a7e6f v8::internal::JSObject::SetPropertyWithCallback(v8::internal::Object*, v8::internal::String*, v8::internal::Object*, v8::internal::JSObject*, v8::internal::StrictModeFlag)
5   0x3f0ab7a7 v8::internal::JSObject::SetPropertyForResult(v8::internal::LookupResult*, v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag)
6   0x3f0a634b v8::internal::JSReceiver::SetProperty(v8::internal::LookupResult*, v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag)
7   0x3f0a77c1 v8::internal::JSReceiver::SetProperty(v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag)
8   0x3f00b832 v8::internal::StoreIC::Store(v8::internal::InlineCacheState, v8::internal::StrictModeFlag, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::String>, v8::internal::Handle<v8::internal::Object>)
9   0x3f00edf9 v8::internal::StoreIC_Miss(v8::internal::Arguments, v8::internal::Isolate*)
10  0x55208736
[2856:-1601178304:1395648566205:ERROR:process_util_posix.cc(142)] Received signal 11
	0   DumpRenderTree                      0x3e0bbc6f base::debug::StackTrace::StackTrace() + 63
	1   DumpRenderTree                      0x3e0bbc0b base::debug::StackTrace::StackTrace() + 43
	2   DumpRenderTree                      0x3e753e17 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295
	3   libSystem.B.dylib                   0x9651a05b _sigtramp + 43
	4   ???                                 0xffffffff 0x0 + 4294967295
	5   DumpRenderTree                      0x40d1b241 WebCore::AudioPannerNode::setPanningModel(unsigned short) + 161
	6   DumpRenderTree                      0x410ecc2c WebCore::AudioPannerNodeInternal::panningModelAttrSetter(v8::Local<v8::String>, v8::Local<v8::Value>, v8::AccessorInfo const&) + 124
	7   DumpRenderTree                      0x3f0a7e6f v8::internal::JSObject::SetPropertyWithCallback(v8::internal::Object*, v8::internal::String*, v8::internal::Object*, v8::internal::JSObject*, v8::internal::StrictModeFlag) + 943
	8   DumpRenderTree                      0x3f0ab7a7 v8::internal::JSObject::SetPropertyForResult(v8::internal::LookupResult*, v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag) + 1751
	9   DumpRenderTree                      0x3f0a634b v8::internal::JSReceiver::SetProperty(v8::internal::LookupResult*, v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag) + 235
	10  DumpRenderTree                      0x3f0a77c1 v8::internal::JSReceiver::SetProperty(v8::internal::String*, v8::internal::Object*, PropertyAttributes, v8::internal::StrictModeFlag) + 161
	11  DumpRenderTree                      0x3f00b832 v8::internal::StoreIC::Store(v8::internal::InlineCacheState, v8::internal::StrictModeFlag, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::String>, v8::internal::Handle<v8::internal::Object>) + 2658
	12  DumpRenderTree                      0x3f00edf9 v8::internal::StoreIC_Miss(v8::internal::Arguments, v8::internal::Isolate*) + 441
	13  ???                                 0x55208736 0x0 + 1428195126
ax: bbadbeef, bx: 52082961, cx: 54c6582, dx: 54c6582
di: 0, si: 41e00b5a, bp: bfffb6d8, sp: bfffb660, ss: 23, flags: 10282
ip: 4005b955, cs: 1b, ds: 23, es: 23, fs: 0, gs: f

[Raymond Toy]

This is caused at line 57 in platform/audio/Panner.cpp.  The sound field panning model is not implemented, which the new test actually tests.

Chris, what is the correct approach here?  Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model?

[Chris Rogers]

(In reply to comment #1)
> This is caused at line 57 in platform/audio/Panner.cpp.  The sound field panning model is not implemented, which the new test actually tests.
> 
> Chris, what is the correct approach here?  Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model?

The correct approach is to not call ASSERT_NOT_REACHED, which should only be used to assert bugs and logic errors in the C++ code.  Ideally, we would be throwing an exception here, but another option is to log a console message similar to what we do in AudioBufferSourceNode when the deprecated "looping" attribute is used.

Even better yet, we'll implement the SOUNDFIELD algorithm!

[Raymond Toy]

(In reply to comment #2)
> (In reply to comment #1)
> > This is caused at line 57 in platform/audio/Panner.cpp.  The sound field panning model is not implemented, which the new test actually tests.
> > 
> > Chris, what is the correct approach here?  Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model?
> 
> The correct approach is to not call ASSERT_NOT_REACHED, which should only be used to assert bugs and logic errors in the C++ code.  Ideally, we would be throwing an exception here, but another option is to log a console message similar to what we do in AudioBufferSourceNode when the deprecated "looping" attribute is used.

Ok.  I'll add a log message for this.  The exception part can be handled when https://bugs.webkit.org/show_bug.cgi?id=77235 is addressed.  (Will add a comment there to remind us about the sound field model.)

> 
> Even better yet, we'll implement the SOUNDFIELD algorithm!

Sounds like another bug report to be created. :-)

[Raymond Toy]

Created attachment 124620 [details]
Patch

[Chris Rogers]

Comment on attachment 124620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review

> Source/WebCore/platform/audio/Panner.cpp:57
> +        return nullptr;

We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug#

In other words, we can't just allow m_panner to be set to nullptr and get crashes later on!

[Raymond Toy]

(In reply to comment #5)
> (From update of attachment 124620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review
> 
> > Source/WebCore/platform/audio/Panner.cpp:57
> > +        return nullptr;
> 
> We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug#

Ok.
> 
> In other words, we can't just allow m_panner to be set to nullptr and get crashes later on!

Actually, it doesn't crash.  I tested this with the panner/reverb demo and manually set the panning model to 2 via the javascript console.  Demo continues, just without audio.

[Chris Rogers]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 124620 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review
> > 
> > > Source/WebCore/platform/audio/Panner.cpp:57
> > > +        return nullptr;
> > 
> > We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug#
> 
> Ok.
> > 
> > In other words, we can't just allow m_panner to be set to nullptr and get crashes later on!
> 
> Actually, it doesn't crash.  I tested this with the panner/reverb demo and manually set the panning model to 2 via the javascript console.  Demo continues, just without audio.

But it should fail without side-effects.  It should just completely *ignore* SOUNDFIELD and not disturb the panning from the previous algorithm

[Raymond Toy]

Created attachment 124805 [details]
Patch

[Raymond Toy]

Updated according to review.  AudioPannerNode now does nothing if SOUNDFIELD is selected, except that a warning message is sent to the Javascript console.   Debug builds don't crash anymore.

[WebKit Review Bot]

Comment on attachment 124805 [details]
Patch

Attachment 124805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11389123

New failing tests:
webaudio/panner-set-model.html

[Raymond Toy]

(In reply to comment #10)
> (From update of attachment 124805 [details])
> Attachment 124805 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11389123
> 
> New failing tests:
> webaudio/panner-set-model.html

I don't understand this failure.  The output doesn't show anything about panner-set-model.

[Raymond Toy]

Created attachment 124968 [details]
Update expected results to match actual which now includes a console message

[Raymond Toy]

As discussed with Chris, we would rather fix this issue in bug 77235 and throw an exception instead of silently doing nothing.  When the patch for 77235 has landed, we should be able to close out this bug.

[Raymond Toy]

Bug 77235 has landed.  Closing this.

[Eric Seidel (no email)]

Comment on attachment 124968 [details]
Update expected results to match actual which now includes a console message

Cleared review? from attachment 124968 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Raymond Toy]

(In reply to comment #15)
> (From update of attachment 124968 [details])
> Cleared review? from attachment 124968 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

Sorry.  I closed the bug when bug 77235 landed but forget to remove the review flags.