Bug 145308 – MediaDevices.getUserMedia should put promises in resolve/reject state synchronously

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

The current implementation calls callOnMainThread to reject/resolve the returned promise.
It might be better to reject/resolve directly, still ensuring that webkitGetUserMedia callbacks are called asynchronously, either using promise mechanism or using postTask directly

[youenn fablet]

Created attachment 253979 [details]
Patch

[youenn fablet]

Comment on attachment 253979 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:86
>      return UserMediaRequest::create(context, controller, options, WTF::move(resolveCallback), WTF::move(rejectCallback), ec);

After looking at current implementation, I think resolveCallback and rejectCallback are currently always called asynchronously, which is necessary for webkitGetUserMedia implementation.
But code might change and may break this assumption.
An ASSERT would be ideal here.
Or we can ensure that handleEvent calls are made asynchronous using postTask.
Any thought?

[Adam Bergkvist]

As you say above, the scheduling of calls to handleEvent() must done in the webkitGetUserMedia() case. But I'm a bit confused since this patch removes the "always schedule" case (which is good), but never adds any conditional scheduling.

[youenn fablet]

(In reply to comment #3)
> As you say above, the scheduling of calls to handleEvent() must done in the
> webkitGetUserMedia() case. But I'm a bit confused since this patch removes
> the "always schedule" case (which is good), but never adds any conditional
> scheduling.

I checked again and it seems I was indeed partially wrong.

The success callback is always called asynchronously: the user permission is always requested and it is done inside a callOnMainThread.

But it seems that some error cases in the constraints validation may call the failure callback synchronously.

A slightly sub-optimal but simple approach may be to call handleEvent for error cases inside a postTask. For HandleEvent success callback, we do not need postTask (although it may be safer if code changes a lot).

How about that?

[youenn fablet]

Created attachment 254165 [details]
Emulating promise resolution with postTask

[youenn fablet]

(In reply to comment #5)
> Created attachment 254165 [details]
> Emulating promise resolution with postTask

I changed my mind and went the safe way, using postTask for both success and error callbacks.
There is no real point in trying to optimize an API (webkitGetUerMedia) that is in the process of being replaced.

[WebKit Commit Bot]

Comment on attachment 254165 [details]
Emulating promise resolution with postTask

Clearing flags on attachment: 254165

Committed r185191: <http://trac.webkit.org/changeset/185191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Bergkvist]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 254165 [details]
> > Emulating promise resolution with postTask
> 
> I changed my mind and went the safe way, using postTask for both success and
> error callbacks.

Sounds reasonable. :)

[Adam Bergkvist]

Thanks for taking the time to improve the Promise handling here. :) The RTCPeerConnection API will use Promises as well (together with callbacks for legacy interop) so it's great to get these in better shape.

[youenn fablet]

(In reply to comment #10)
> Thanks for taking the time to improve the Promise handling here. :) The
> RTCPeerConnection API will use Promises as well (together with callbacks for
> legacy interop) so it's great to get these in better shape.

I worked on some additional Promise support improvement lately.
I hope to enable automatic binding generation for AudioContext promise API.
Please have a look at it when starting RTCPeerConnection API implementation.

The case of "Promise + callbacks-for-legacy" APIs may not be as straightforward though.
There is also the case of exceptions rejecting promises that might need some specific support.