Bug 145282 - MediaDevices.getUserMedia should reject promise instead of throwing exceptions
Summary: MediaDevices.getUserMedia should reject promise instead of throwing exceptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 143211
  Show dependency treegraph
 
Reported: 2015-05-21 13:53 PDT by youenn fablet
Modified: 2015-05-29 01:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.28 KB, patch)
2015-05-21 13:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing async promise resolution/rejection changes (9.35 KB, patch)
2015-05-22 02:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (10.64 KB, patch)
2015-05-28 13:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-05-21 13:53:57 PDT
As per http://w3c.github.io/mediacapture-main/archives/20150324/getusermedia.html#dom-mediadevices-getusermedia, all exceptions or errors should be passed to the web application by rejecting the promise.
Comment 1 youenn fablet 2015-05-21 13:59:36 PDT
Created attachment 253541 [details]
Patch
Comment 2 youenn fablet 2015-05-21 23:15:11 PDT
Comment on attachment 253541 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevices.h:59
> +    typedef std::function<void(NavigatorUserMediaError*)> RejectCallback;

Can we resolve/reject with nullptr for getUserMedia?
I guess it is not the case but I am not sure.
We may want to pass parameters as references (& or Ref<>&&) if possible, probably once PassRefPtr<>, RefPtr<>, Ref<> cleaning is done.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:172
> +    RefPtr<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, privateStream);

Can stream be null? Can we use a Ref<> instead?

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:187
>      RefPtr<NavigatorUserMediaError> error = NavigatorUserMediaError::create(NavigatorUserMediaError::constraintNotSatisfiedErrorName(), constraintName);

Can error be null? Ca we use a Ref<> instead?

> LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8
> -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined).

This last test case is strange. Is it what we want to test?
I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})"
Comment 3 youenn fablet 2015-05-22 01:11:42 PDT
Comment on attachment 253541 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83
> +            errorCallback->handleEvent(error);

With the proposed changes, the rejectCallback may be called synchronously now.
The promise implementation ensures that JS callbacks are called asynchronously.
But for webkitGetUserMedia, after looking at http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia-interface-extensions, this is probably an issue.

We may emulate asynchronous promise callback execution with something like:
RefPtr<NavigatorUserMediaError> protectedError = error;
scriptExecutionContext()->postTask([errorCallback, protectedError] {
   errorCallback->handleEvent(protectedError.get());
});
Or think of a better way to do the same thing (handling at promise level just like the spec says).

successCallback might need to have the same treatment for consistency purposes.

Also, this might need a dedicated test case, since it seems the tests do not capture that.
Rethinking about it, splitting the patch in two might make sense.

I will wait for some feedback before doing anything, maybe I am overlooking things.
Comment 4 Adam Bergkvist 2015-05-22 01:22:02 PDT
(In reply to comment #2)
> Comment on attachment 253541 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253541&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaDevices.h:59
> > +    typedef std::function<void(NavigatorUserMediaError*)> RejectCallback;
> 
> Can we resolve/reject with nullptr for getUserMedia?
> I guess it is not the case but I am not sure.
> We may want to pass parameters as references (& or Ref<>&&) if possible,
> probably once PassRefPtr<>, RefPtr<>, Ref<> cleaning is done.

No. Always a MediaSteram or an error.

> 
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:172
> > +    RefPtr<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, privateStream);
> 
> Can stream be null? Can we use a Ref<> instead?
> 
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:187
> >      RefPtr<NavigatorUserMediaError> error = NavigatorUserMediaError::create(NavigatorUserMediaError::constraintNotSatisfiedErrorName(), constraintName);
> 
> Can error be null? Ca we use a Ref<> instead?
> 

I believe the answer is no to both questions above.

> > LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8
> > -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined).
> 
> This last test case is strange. Is it what we want to test?
> I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})"

MediaDevices.getUserMedia() is a rather late addition to the Media Capture and Streams spec [1]. It's sibling, on Navigator, has been along for quite some time, so when we decided to expose the Promised-based version on the MediaDevices object, we said that implementations should not vendor prefix it since the functionality is rather mature. So this test is intended to check that.

[1] https://w3c.github.io/mediacapture-main/
Comment 5 Adam Bergkvist 2015-05-22 01:28:22 PDT
> > Can error be null? Ca we use a Ref<> instead?
> > 
> 
> I believe the answer is no to both questions above.

I realize my answer was a bit ambiguous. :) It can't be null, and yes, we could use Ref<> instead.
Comment 6 Adam Bergkvist 2015-05-22 01:45:13 PDT
(In reply to comment #3)
> Comment on attachment 253541 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253541&action=review
> 
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83
> > +            errorCallback->handleEvent(error);
> 
> With the proposed changes, the rejectCallback may be called synchronously
> now.
> The promise implementation ensures that JS callbacks are called
> asynchronously.
> But for webkitGetUserMedia, after looking at
> http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia-
> interface-extensions, this is probably an issue.
> 
> We may emulate asynchronous promise callback execution with something like:
> RefPtr<NavigatorUserMediaError> protectedError = error;
> scriptExecutionContext()->postTask([errorCallback, protectedError] {
>    errorCallback->handleEvent(protectedError.get());
> });
> Or think of a better way to do the same thing (handling at promise level
> just like the spec says).
> 
> successCallback might need to have the same treatment for consistency
> purposes.

The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern?
Comment 7 youenn fablet 2015-05-22 02:42:29 PDT
Comment on attachment 253541 [details]
Patch

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

>>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83
>>> +            errorCallback->handleEvent(error);
>> 
>> With the proposed changes, the rejectCallback may be called synchronously now.
>> The promise implementation ensures that JS callbacks are called asynchronously.
>> But for webkitGetUserMedia, after looking at http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia-interface-extensions, this is probably an issue.
>> 
>> We may emulate asynchronous promise callback execution with something like:
>> RefPtr<NavigatorUserMediaError> protectedError = error;
>> scriptExecutionContext()->postTask([errorCallback, protectedError] {
>>    errorCallback->handleEvent(protectedError.get());
>> });
>> Or think of a better way to do the same thing (handling at promise level just like the spec says).
>> 
>> successCallback might need to have the same treatment for consistency purposes.
>> 
>> Also, this might need a dedicated test case, since it seems the tests do not capture that.
>> Rethinking about it, splitting the patch in two might make sense.
>> 
>> I will wait for some feedback before doing anything, maybe I am overlooking things.
> 
> The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern?

Yes and no ;)
Using callOnMainThread would solve the concern for webkitGetUserMedia (although postTask would be better as it is what promise implementation uses for calling JS callbacks).

But it creates an issue for MediaDevices.getUserMedia() as the postTask/callOnMainThread call is not needed at all to resolve/reject promise.
The JS promise implementation is already doing that work.
This makes things less efficient and less consistent.

I will split that as a separate patch since it is less straightforward than I thought it would be.
Let's first reject exceptions.

>>> LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8
>>> -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined).
>> 
>> This last test case is strange. Is it what we want to test?
>> I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})"
> 
> MediaDevices.getUserMedia() is a rather late addition to the Media Capture and Streams spec [1]. It's sibling, on Navigator, has been along for quite some time, so when we decided to expose the Promised-based version on the MediaDevices object, we said that implementations should not vendor prefix it since the functionality is rather mature. So this test is intended to check that.
> 
> [1] https://w3c.github.io/mediacapture-main/

OK.
Asserting that navigator.mediaDevices.webkitGetUserMedia is undefined might be clearer then?
Comment 8 youenn fablet 2015-05-22 02:56:16 PDT
Created attachment 253586 [details]
Removing async promise resolution/rejection changes
Comment 9 youenn fablet 2015-05-22 03:06:05 PDT
> > The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern?
> 
> Yes and no ;)
> Using callOnMainThread would solve the concern for webkitGetUserMedia
> (although postTask would be better as it is what promise implementation uses
> for calling JS callbacks).
> 
> But it creates an issue for MediaDevices.getUserMedia() as the
> postTask/callOnMainThread call is not needed at all to resolve/reject
> promise.
> The JS promise implementation is already doing that work.
> This makes things less efficient and less consistent.

You can see an illustration of the issue in the -expected.txt generated by the latest patch.
The order of the callback execution is not really what we would expect.

First, all promise rejection callbacks are called asynchronously which is good.

But the empty dictionary test is displayed before the {audio:true} test for instance.

Reason is the custom binding is implemented with just one postTask.
The rejection done in UserMediaRequest is implemented with one callOnnMainThread followed by one postTask.
Comment 10 Adam Bergkvist 2015-05-22 04:42:37 PDT
(In reply to comment #7)
> OK.
> Asserting that navigator.mediaDevices.webkitGetUserMedia is undefined might
> be clearer then?

Yes, that's better.

(In reply to comment #9)
> > > The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern?
> > 
> > Yes and no ;)
> > Using callOnMainThread would solve the concern for webkitGetUserMedia
> > (although postTask would be better as it is what promise implementation uses
> > for calling JS callbacks).
> > 
> > But it creates an issue for MediaDevices.getUserMedia() as the
> > postTask/callOnMainThread call is not needed at all to resolve/reject
> > promise.
> > The JS promise implementation is already doing that work.
> > This makes things less efficient and less consistent.
> 
> You can see an illustration of the issue in the -expected.txt generated by
> the latest patch.
> The order of the callback execution is not really what we would expect.
> 
> First, all promise rejection callbacks are called asynchronously which is
> good.
> 
> But the empty dictionary test is displayed before the {audio:true} test for
> instance.
> 
> Reason is the custom binding is implemented with just one postTask.
> The rejection done in UserMediaRequest is implemented with one
> callOnnMainThread followed by one postTask.

The double scheduling is not ideal indeed. I believe your proposal to move the scheduling needed for the callback case into the lambda functions does the trick right?
Comment 11 youenn fablet 2015-05-22 05:02:27 PDT
(In reply to comment #10)
> > Reason is the custom binding is implemented with just one postTask.
> > The rejection done in UserMediaRequest is implemented with one
> > callOnnMainThread followed by one postTask.
> 
> The double scheduling is not ideal indeed. I believe your proposal to move
> the scheduling needed for the callback case into the lambda functions does
> the trick right?

Yes.
I filed bug 145308 for that purpose.
Comment 12 youenn fablet 2015-05-22 05:07:24 PDT
As a side note, the spec http://w3c.github.io/mediacapture-main/archives/20150324/getusermedia.html calls somehow for that double scheduing.
In particular, it says that step 3 should be done asynchronously.
Might be worth some spec editing maybe.
Should I file a github issue?
Comment 13 Adam Bergkvist 2015-05-22 05:28:37 PDT
Please, file an issue.
Comment 14 Darin Adler 2015-05-26 11:44:22 PDT
Comment on attachment 253586 [details]
Removing async promise resolution/rejection changes

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

> Source/WebCore/Modules/mediastream/MediaDevices.h:59
> +    typedef std::function<void(MediaStream*)> ResolveCallback;
> +    typedef std::function<void(NavigatorUserMediaError*)> RejectCallback;

Why pointers and not references? Can these ever be null?
Comment 15 youenn fablet 2015-05-26 14:13:14 PDT
Comment on attachment 253586 [details]
Removing async promise resolution/rejection changes

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

>> Source/WebCore/Modules/mediastream/MediaDevices.h:59
>> +    typedef std::function<void(NavigatorUserMediaError*)> RejectCallback;
> 
> Why pointers and not references? Can these ever be null?

Thanks for reminding me of this point.
I checked with Adam and he said references are fine.
I will clean this when landing.
Comment 16 youenn fablet 2015-05-27 23:42:37 PDT
(In reply to comment #13)
> Please, file an issue.

Filed https://github.com/w3c/mediacapture-main/issues/174 for that purpose.
Comment 17 youenn fablet 2015-05-28 13:26:39 PDT
Created attachment 253858 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2015-05-29 01:10:20 PDT
Comment on attachment 253858 [details]
Patch for landing

Clearing flags on attachment: 253858

Committed r184984: <http://trac.webkit.org/changeset/184984>
Comment 19 WebKit Commit Bot 2015-05-29 01:10:24 PDT
All reviewed patches have been landed.  Closing bug.