Bug 233722

Summary: DisplayCaptureSource doesn't initialize base class correctly
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebRTCAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=235221
Attachments:
Description Flags
Patch
none
Patch none

Description Eric Carlson 2021-12-01 13:28:59 PST
UserMediaCaptureManagerProxy doesn't pass the site hash salt the display capture factory, so RealtimeMediaSource::m_hashedID is empty.
Comment 1 Eric Carlson 2021-12-01 14:05:40 PST
Created attachment 445614 [details]
Patch
Comment 2 youenn fablet 2021-12-01 22:53:34 PST
Comment on attachment 445614 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No change in functionality.

Is the goal to expose a deviceId in settings? If so, we should call setDeviceId in DisplayCaptureSourceCocoa::settings() and add a test.
If not, why do we care about having an empty hashed ID?

Looking at other browsers, we are doing like Firefox (no deviceId) but not like Chrome.
I guess the spec should be clear about that.

Also RealtimeMediaSource::hashedId is checking that m_hashedId is not empty if device is not screen/window.
I guess we could update this check (though other sources might still have empty hashedId, canvas or WebAudio for instance).

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:126
> +    void getDisplayMediaDevices(const MediaStreamRequest&, String&&, Vector<DeviceInfo>&, String&);

Would be worth changing to Expected<Vector<DeviceInfo>, String> getDisplayMediaDevices(..) at some point (probably not this patch though).
Comment 3 Eric Carlson 2021-12-02 15:00:18 PST
Created attachment 445777 [details]
Patch
Comment 4 Eric Carlson 2021-12-02 15:05:04 PST
(In reply to youenn fablet from comment #2)
> Comment on attachment 445614 [details]
> 
> Is the goal to expose a deviceId in settings? If so, we should call
> setDeviceId in DisplayCaptureSourceCocoa::settings() and add a test.
> If not, why do we care about having an empty hashed ID?
> 

Good point, I added a test.

> Looking at other browsers, we are doing like Firefox (no deviceId) but not
> like Chrome.
> I guess the spec should be clear about that.
> 
> Also RealtimeMediaSource::hashedId is checking that m_hashedId is not empty
> if device is not screen/window.
> I guess we could update this check (though other sources might still have
> empty hashedId, canvas or WebAudio for instance).
> 

I updated the check.
Comment 5 EWS 2021-12-03 08:36:06 PST
Committed r286491 (244830@main): <https://commits.webkit.org/244830@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445777 [details].
Comment 6 Radar WebKit Bug Importer 2021-12-03 08:37:23 PST
<rdar://problem/86019589>