Bug 145282 – MediaDevices.getUserMedia should reject promise instead of throwing exceptions

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 253541 [details]
Patch

[youenn fablet]

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})"

[youenn fablet]

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.

[Adam Bergkvist]

(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/

[Adam Bergkvist]

> > 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.

[Adam Bergkvist]

(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?

[youenn fablet]

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?

[youenn fablet]

Created attachment 253586 [details]
Removing async promise resolution/rejection changes

[youenn fablet]

> > 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.

[Adam Bergkvist]

(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?

[youenn fablet]

(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.

[youenn fablet]

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?

[Adam Bergkvist]

Please, file an issue.

[Darin Adler]

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?

[youenn fablet]

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.

[youenn fablet]

(In reply to comment #13)
> Please, file an issue.

Filed https://github.com/w3c/mediacapture-main/issues/174 for that purpose.

[youenn fablet]

Created attachment 253858 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 253858 [details]
Patch for landing

Clearing flags on attachment: 253858

Committed r184984: <http://trac.webkit.org/changeset/184984>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.