Bug 190142 – [MediaStream] RealtimeMediaSource should be able to vend hashed IDs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

RealtimeMediaSource should know its hash salt so it can return a properly hashed device ID.

[Radar WebKit Bug Importer]

<rdar://problem/44911109>

[Eric Carlson]

Created attachment 351273 [details]
Patch

[Eric Carlson]

Created attachment 351292 [details]
Updated patch

[Eric Carlson]

Created attachment 351324 [details]
Updated patchh

[Eric Carlson]

Created attachment 351346 [details]
Updated patchh

[youenn fablet]

Comment on attachment 351346 [details]
Updated patchh

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

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:122
>          document.setDeviceIDHashSalt(deviceIdentifierHashSalt);

Maybe move the ASSERT in document.setDeviceIDHashSalt.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:58
> +    else

ASSERT(!m_idHashSalt.isEmpty());

> Source/WebCore/platform/mediastream/RealtimeMediaSourceFactory.h:38
>  struct CaptureSourceOrError;

CaptureSourceOrError before.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:245
> +        capabilities.setDeviceId(hashedId());

Probably unneeded, let's remove it now or in a follow-up.

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:128
> +void UserMediaCaptureManagerProxy::createMediaSourceForCaptureDeviceWithConstraints(uint64_t id, const CaptureDevice& device, WebCore::RealtimeMediaSource::Type type, const String& hashSalt, const MediaConstraints& constraints, bool& succeeded, String& invalidConstraints, WebCore::RealtimeMediaSourceSettings& settings)

String&& hashSalt?

[Eric Carlson]

Comment on attachment 351346 [details]
Updated patchh

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

>> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:122
>>          document.setDeviceIDHashSalt(deviceIdentifierHashSalt);
> 
> Maybe move the ASSERT in document.setDeviceIDHashSalt.

Good idea, fixed.

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:58
>> +    else
> 
> ASSERT(!m_idHashSalt.isEmpty());

That doesn't work because remote tracks must pass ID but don't have (or need) a hash salt. I did add an ASSERT to RealtimeMediaSource::hashedId because it should only be called on sources that have a valid salt.

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceFactory.h:38
>>  struct CaptureSourceOrError;
> 
> CaptureSourceOrError before.

Fixed

>> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:128
>> +void UserMediaCaptureManagerProxy::createMediaSourceForCaptureDeviceWithConstraints(uint64_t id, const CaptureDevice& device, WebCore::RealtimeMediaSource::Type type, const String& hashSalt, const MediaConstraints& constraints, bool& succeeded, String& invalidConstraints, WebCore::RealtimeMediaSourceSettings& settings)
> 
> String&& hashSalt?

Fixed.

[Eric Carlson]

Created attachment 351364 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 351364 [details]
Patch for landing

Clearing flags on attachment: 351364

Committed r236730: <https://trac.webkit.org/changeset/236730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r236730 for reason:

This caused a consistent crash in test http/tests/media/media-stream/get-display-media-prompt.html.

Committed r236855: <https://trac.webkit.org/changeset/236855>

[Matt Lewis]

Created attachment 351637 [details]
get-display-media-prompt-crash-log.txt

Attaching the crash log for the assertion this hit as well as sample builds:

ASSERTION FAILED: !m_text.isNull()
/Volumes/Data/slave/sierra-debug/build/Source/WebCore/platform/graphics/TextRun.h(60) : WebCore::TextRun::TextRun(const WTF::String &, float, float, ExpansionBehavior, WebCore::TextDirection, bool, bool)
1   0x125bd9209 WTFCrash
2   0x116ae9fdb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x119022f49 WebCore::TextRun::TextRun(WTF::String const&, float, float, unsigned int, WebCore::TextDirection, bool, bool)
4   0x119011d1f WebCore::TextRun::TextRun(WTF::String const&, float, float, unsigned int, WebCore::TextDirection, bool, bool)
5   0x119a04cc3 WebCore::MockRealtimeVideoSource::drawText(WebCore::GraphicsContext&)
6   0x119a02786 WebCore::MockRealtimeVideoSource::generateFrame()
7   0x119a15405 WTF::RunLoop::Timer<WebCore::MockRealtimeVideoSource>::fired()
8   0x125c59d11 WTF::RunLoop::TimerBase::timerFired(__CFRunLoopTimer*, void*)
9   0x7fff9a3d0e04 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
10  0x7fff9a3d0a93 __CFRunLoopDoTimer
11  0x7fff9a3d05ea __CFRunLoopDoTimers
12  0x7fff9a3c7fc1 __CFRunLoopRun
13  0x7fff9a3c7544 CFRunLoopRunSpecific
14  0x7fff99926ebc RunCurrentEventLoopInMode
15  0x7fff99926cf1 ReceiveNextEventCommon
16  0x7fff99926b26 _BlockUntilNextEventMatchingListInModeWithFilter
17  0x7fff97ebda54 _DPSNextEvent
18  0x7fff986397ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
19  0x7fff97eb23db -[NSApplication run]
20  0x7fff97e7ce0e NSApplicationMain
21  0x7fffb01ea8c7 _xpc_objc_main
22  0x7fffb01e92e4 xpc_main
23  0x10f44e08e WebKit::XPCServiceMain(int, char const**)
24  0x10f44e382 main
25  0x7fffaff91235 start
26  0x1
LEAK: 1 WebPageProxy


http/tests/media/media-stream/get-display-media-prompt.html is crashing consistently on macOS Sierra and HighSierra Debug and is getting an invalid crash with macOS Mojave Debug.
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fmedia%2Fmedia-stream%2Fget-display-media-prompt.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r236828%20(8227)/results.html

[Matt Lewis]

I was able to reproduce this consistently with run-webkit-tests --debug http/tests/media/media-stream/get-display-media-prompt.html after the revision. before this revision I wasn't able to reproduce the assertion.

[Eric Carlson]

Created attachment 351678 [details]
Fix test crash

[WebKit Commit Bot]

Comment on attachment 351678 [details]
Fix test crash

Clearing flags on attachment: 351678

Committed r236877: <https://trac.webkit.org/changeset/236877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.