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
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?
(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!
(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. :-)
Created attachment 124620 [details] Patch
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!
(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.
(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
Created attachment 124805 [details] Patch
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.
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
(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.
Created attachment 124968 [details] Update expected results to match actual which now includes a console message
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.
Bug 77235 has landed. Closing this.
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).
(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.