Bug 60174 – Fix getUserMedia callback bindings for JSC

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Bergkvist]

The existing JSC bindings does not work.

[Adam Bergkvist]

Created attachment 92255 [details]
Proposed patch

[Early Warning System Bot]

Attachment 92255 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8557559

[Build Bot]

Attachment 92255 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554559

[Alexey Proskuryakov]

> The existing JSC bindings does not work.
+        No new functionality to be tested.

These sentences seem to be contradictory.

[Collabora GTK+ EWS bot]

Attachment 92255 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8557645

[WebKit Review Bot]

Attachment 92255 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8571290

[Adam Bergkvist]

(In reply to comment #4)
> > The existing JSC bindings does not work.
> +        No new functionality to be tested.
> 
> These sentences seem to be contradictory.

You're right. What I meant to say was that the functionality is already covered by existing tests. Although the tests pass, I had to tweak one of them since the expected results differed between JSC and V8 (see http://webkit.org/b/60391).

[Adam Bergkvist]

Created attachment 92619 [details]
Updated patch

The problem can be solved while still using generated bindings. However, the currently generated bindings fail due to a callback argument not being translated correctly resulting in the handleEvent() function not being overridden as it should (argument mismatch between int and int*). To avoid updating the JSC bindings generator, the fix was to change the failing "dummy" argument type to DOMString since primitive number types are not supported as callback arguments. Also, in the webkitGetUserMedia() custom binding the argument count is checked before trying to convert the first argument (fixes a failure in the argument-types test).

[Darin Adler]

Comment on attachment 92619 [details]
Updated patch

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

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:43
> +    if (exec->argumentCount() < 2) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
> +        return jsUndefined();
> +    }

The desired behavior here, then, is that undefined turns into "undefined" for options, but that a missing argument yields TYPE_MISMATCH_ERR?

Similarly, I’m surprised that undefined for successCallback has different behavior than a missing argument, given that the missing argument behavior is TYPE_MISMATCH_ERR.

I’m a little surprised by these. Are they really covered by existing tests? I don’t see tests that are passing undefined.

[Adam Bergkvist]

Created attachment 93112 [details]
Updated patch

I agree. I made the mistake of trying to get an existing test to pass instead of checking the specification for the correct behavior. According to the specification there should never be any type mismatch errors and getUserMedia should fail silently if successCallback is null. If options end up being "undefined" a NOT_SUPPORTED_ERR exception will be thrown at a later stage. I have created a separate bug to fix this (see http://webkit.org/b/60622).

[Darin Adler]

Comment on attachment 93112 [details]
Updated patch

This change seems OK. I don’t fully understand the context, but clearly it will do no harm.

[Leandro Graciá Gil]

I think this has been caused by my arbitrary decision of using int as the dummy type. My apologies if it caused problems in JSC. I'll keep it in mind in case I see myself in a similar situation in the future.

In any case, this code you're trying to patch doesn't have dummies anymore since the bug 56666 introducing the GeneratedStream class has been finally landed. This makes the patch unnecessary at this point.

Layout tests for getUserMedia should be working now with the expected results in both JSC and V8. In case they don't, please don't hesitate to contact me or [e-mail].

Thanks for contributing to this!

[Tony Gentilcore]

r-/cq- as this won't merge and isn't necessary after http://trac.webkit.org/changeset/87150.

[Adam Bergkvist]

(In reply to comment #12)
> I think this has been caused by my arbitrary decision of using int as the dummy type. My apologies if it caused problems in JSC. I'll keep it in mind in case I see myself in a similar situation in the future.
>

The JSC bindings generator didn't work too well with the int. I think it should be avoided since it's not a primitive type in WebIDL, although I've seen it used in other places in WebKit as well.

> In any case, this code you're trying to patch doesn't have dummies anymore since the bug 56666 introducing the GeneratedStream class has been finally landed. This makes the patch unnecessary at this point.

It was obviously temporary code but the fix turned out to be so tiny so I thought it would be nice to be able to run the tests (on the GTK port) asap. 

> Layout tests for getUserMedia should be working now with the expected results in both JSC and V8. In case they don't, please don't hesitate to contact me or [e-mail].

I still get the same mismatching diff against the expected results (see http://webkit.org/b/60391).

I've noticed another difference between the V8 and JSC bindings regarding how the exception code is handled. It is not initialized (to 0) at creation in the JSC bindings, but I've seen that you've added an ec = 0; further down in the controller code to handle the consequences.

[Tommy Widenflycht]

> I've noticed another difference between the V8 and JSC bindings regarding how the exception code is handled. It is not initialized (to 0) at creation in the JSC bindings, but I've seen that you've added an ec = 0; further down in the controller code to handle the consequences.

I'll take care of the initialization problem, thanks for pointing it out!
Please advise if you want me to create a new bug for it instead of reusing this one.

[Tommy Widenflycht]

Created attachment 95423 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95423 [details]:

http/tests/websocket/tests/error-detect.html bug 54012 (author: [e-mail])
http/tests/websocket/tests/handshake-fail-by-no-upgrade-header.html bug 61784 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95423 [details]
Patch

Clearing flags on attachment: 95423

Committed r87725: <http://trac.webkit.org/changeset/87725>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.