Bug 104188 – Implement OfflineAudioContext constructor

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Implement OfflineAudioContext constructor

[Chris Rogers]

Created attachment 177873 [details]
Patch

[Adam Barth]

Comment on attachment 177873 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:42
> +v8::Handle<v8::Value> V8OfflineAudioContext::constructorCallbackCustom(const v8::Arguments& args)

This looks like it can be auto generated.  You'll just need to do the bounds checking in WebCore and use an ec to throw an exception.

[Chris Rogers]

Created attachment 177905 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177905 [details]
Patch

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

[Kentaro Hara]

Comment on attachment 177905 [details]
Patch

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

> Source/WebCore/Modules/webaudio/DOMWindowWebAudio.idl:33
>      [JSCustomGetter, V8EnabledAtRuntime] attribute AudioContextConstructor webkitAudioContext;
> +    [JSCustomGetter, V8EnabledAtRuntime] attribute OfflineAudioContextConstructor webkitOfflineAudioContext;

Can't you remove JSCustomGetter by improving CodeGeneratorJS.pm?

[Kentaro Hara]

Comment on attachment 177905 [details]
Patch

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

> Source/WebCore/bindings/js/JSOfflineAudioContextCustom.cpp:75
> +    if (numberOfChannels <= 0 || numberOfChannels > 10)
> +        return throwVMError(exec, createSyntaxError(exec, "Invalid number of channels"));
> +
> +    if (numberOfFrames <= 0)
> +        return throwVMError(exec, createSyntaxError(exec, "Invalid number of frames"));
> +
> +    if (sampleRate <= 0)
> +        return throwVMError(exec, createSyntaxError(exec, "Invalid sample rate"));

Should these really be JavaScript errors not DOM exceptions? (Would you tell me the spec link?) If they are DOM exceptions, you can throw them in OfflineAudioContext::create(). Then you can completely remove custom code for constructors.

> Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:65
> +    int32_t numberOfChannels = toInt32(args[0], ok);
> +    if (!ok || numberOfChannels <= 0 || numberOfChannels > 10)
> +        return throwError(v8SyntaxError, "Invalid number of channels", args.GetIsolate());
> +
> +    int32_t numberOfFrames = toInt32(args[1], ok);
> +    if (!ok || numberOfFrames <= 0)
> +        return throwError(v8SyntaxError, "Invalid number of frames", args.GetIsolate());
> +
> +    float sampleRate = toFloat(args[2]);
> +    if (sampleRate <= 0)
> +        return throwError(v8SyntaxError, "Invalid sample rate", args.GetIsolate());

Ditto.

[Chris Rogers]

(In reply to comment #2)
> (From update of attachment 177873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177873&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:42
> > +v8::Handle<v8::Value> V8OfflineAudioContext::constructorCallbackCustom(const v8::Arguments& args)
> 
> This looks like it can be auto generated.  You'll just need to do the bounds checking in WebCore and use an ec to throw an exception.

Adam, could you please provide some more details about *how* this can be auto-generated?  I know how to do this kind of thing for methods (such as AudioContext methods), but don't understand how to have the DOMWindow *constructor* auto-generated to have the appropriate arguments which will call into OfflineAudioContext::create(...)

Thanks!

[Kentaro Hara]

(In reply to comment #7)
> > This looks like it can be auto generated.  You'll just need to do the bounds checking in WebCore and use an ec to throw an exception.
> 
> Adam, could you please provide some more details about *how* this can be auto-generated?

(Adam is on vacation.)

You can auto-generate a constructor by a [Constructor] IDL atribute (See https://trac.webkit.org/wiki/WebKitIDL#Constructor and WebKit IDL files that are using [Constructor] (e.g. WebSocket.idl)).

In your case, the tricky part is that you need to check parameter values in the constructor. As I commented above, if you can raise DOM exceptions instead of JavaScript errors, you can just move the value checks into WebCore code. Then the [Constructor] IDL attribute will generate the constructor. Otherwise (if you really need to throw JavaScript errors for the invalid values), you can improve code generators so that they can auto-generate the value check code.

Would you tell me the spec please? I'd like to check whether the constructor should throw DOM exceptions or JavaScript errors.

[Chris Rogers]

(In reply to comment #8)
> (In reply to comment #7)
> > > This looks like it can be auto generated.  You'll just need to do the bounds checking in WebCore and use an ec to throw an exception.
> > 
> > Adam, could you please provide some more details about *how* this can be auto-generated?
> 
> (Adam is on vacation.)
> 
> You can auto-generate a constructor by a [Constructor] IDL atribute (See https://trac.webkit.org/wiki/WebKitIDL#Constructor and WebKit IDL files that are using [Constructor] (e.g. WebSocket.idl)).
> 
> In your case, the tricky part is that you need to check parameter values in the constructor. As I commented above, if you can raise DOM exceptions instead of JavaScript errors, you can just move the value checks into WebCore code. Then the [Constructor] IDL attribute will generate the constructor. Otherwise (if you really need to throw JavaScript errors for the invalid values), you can improve code generators so that they can auto-generate the value check code.
> 
> Would you tell me the spec please? I'd like to check whether the constructor should throw DOM exceptions or JavaScript errors.

Thanks Kentaro, this can throw DOM exceptions so hopefully I can take advantage of the technique you recommend.  This will be going into the spec over the weekend...

[Chris Rogers]

Created attachment 178662 [details]
Patch

[Kentaro Hara]

Comment on attachment 178662 [details]
Patch

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

Looks great! No custom code:)

> Source/WebCore/Modules/webaudio/OfflineAudioContext.idl:28
> +    JSGenerateToJSObject,

Is this needed?

[WebKit Review Bot]

Comment on attachment 178662 [details]
Patch

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

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
inspector-protocol/nmi-webaudio-leak-test.html
inspector-protocol/nmi-webaudio.html

[Chris Rogers]

Committed r137398: <http://trac.webkit.org/changeset/137398>

[Kentaro Hara]

Rolled out the patch in r137422 because a bunch of web audio tests hit ASSERTs() in the Debug build.

crogers: would you check it?

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fbiquad-highpass.html

[Chris Rogers]

Reopening to attach new patch.

[Chris Rogers]

Created attachment 179109 [details]
Patch

[Chris Rogers]

(In reply to comment #14)
> Rolled out the patch in r137422 because a bunch of web audio tests hit ASSERTs() in the Debug build.
> 
> crogers: would you check it?
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fbiquad-highpass.html

Sorry about that Kentaro, it turns out that I needed the "EventTarget" keyword in OfflineAudioContext.idl to match AudioContext.idl

This fixes the ASSERTs in the Debug build layout tests.

New patch uploaded with "EventTarget"

[Kentaro Hara]

Comment on attachment 179109 [details]
Patch

Thanks for the fix.

[WebKit Review Bot]

Comment on attachment 179109 [details]
Patch

Clearing flags on attachment: 179109

Committed r137516: <http://trac.webkit.org/changeset/137516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Note that we sometimes hit the following assertion after this patch:
STDERR: ASSERTION FAILED: !m_finishedNodes.size()
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/Modules/webaudio/AudioContext.cpp(198) : virtual WebCore::AudioContext::~AudioContext()
STDERR: 1   0x7f27f144c1b6 WebCore::AudioContext::~AudioContext()
STDERR: 2   0x7f27f1474430 WebCore::OfflineAudioContext::~OfflineAudioContext()
STDERR: 3   0x7f27f1474468 WebCore::OfflineAudioContext::~OfflineAudioContext()
STDERR: 4   0x7f27f144fbcc WTF::ThreadSafeRefCounted<WebCore::AudioContext>::deref()
STDERR: 5   0x7f27f24235a8 WebCore::JSAudioContext::releaseImpl()
STDERR: 6   0x7f27f2443370 WebCore::JSOfflineAudioContextOwner::finalize(JSC::Handle<JSC::Unknown>, void*)
STDERR: 7   0x7f27eb741ac9 JSC::WeakBlock::finalize(JSC::WeakImpl*)
STDERR: 8   0x7f27eb7414bf JSC::WeakBlock::sweep()
STDERR: 9   0x7f27eb7406b6 JSC::WeakSet::sweep()
STDERR: 10  0x7f27eb738965 JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode)
STDERR: 11  0x7f27eb73b2d3 JSC::Sweep::operator()(JSC::MarkedBlock*)
STDERR: 12  0x7f27eb73c5f5 void JSC::MarkedAllocator::forEachBlock<JSC::Sweep>(JSC::Sweep&)
STDERR: 13  0x7f27eb73c111 JSC::Sweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::Sweep>(JSC::Sweep&)
STDERR: 14  0x7f27eb73b93b JSC::Sweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::Sweep>()
STDERR: 15  0x7f27eb73a641 JSC::MarkedSpace::sweep()
STDERR: 16  0x7f27eb729b72 JSC::Heap::collect(JSC::Heap::SweepToggle)
STDERR: 17  0x7f27eb729889 JSC::Heap::collectAllGarbage()
STDERR: 18  0x7f27f233feac
STDERR: 19  0x7f27f233ffa2 WebCore::GCController::gcTimerFired(WebCore::Timer<WebCore::GCController>*)
STDERR: 20  0x7f27f2340232 WebCore::Timer<WebCore::GCController>::fired()
STDERR: 21  0x7f27f1daf1aa WebCore::ThreadTimers::sharedTimerFiredInternal()
STDERR: 22  0x7f27f1daf0cb WebCore::ThreadTimers::sharedTimerFired()
STDERR: 23  0x7f27f286f625
STDERR: 24  0x7f27ee7dd33e _ecore_timer_expired_call
STDERR: 25  0x7f27ee7dd50b _ecore_timer_expired_timers_call
STDERR: 26  0x7f27ee7da421
STDERR: 27  0x7f27ee7daab7 ecore_main_loop_begin
STDERR: 28  0x7f27f286dee7 WebCore::RunLoop::run()
STDERR: 29  0x7f27f6353a84 WebProcessMainEfl
STDERR: 30  0x400804 main
STDERR: 31  0x7f27f53a876d __libc_start_main

I filed bug 105870 to track the issue.

[Adam Barth]

Comment on attachment 179109 [details]
Patch

Thanks for removing the custom bindings code!