Bug 190142

Summary: [MediaStream] RealtimeMediaSource should be able to vend hashed IDs
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebRTCAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jlewis3, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=190284
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patchh
none
Updated patchh
none
Patch for landing
none
get-display-media-prompt-crash-log.txt
none
Fix test crash none

Description Eric Carlson 2018-10-01 09:49:27 PDT
RealtimeMediaSource should know its hash salt so it can return a properly hashed device ID.
Comment 1 Radar WebKit Bug Importer 2018-10-01 09:50:05 PDT
<rdar://problem/44911109>
Comment 2 Eric Carlson 2018-10-01 11:10:10 PDT
Created attachment 351273 [details]
Patch
Comment 3 Eric Carlson 2018-10-01 13:24:07 PDT
Created attachment 351292 [details]
Updated patch
Comment 4 Eric Carlson 2018-10-01 17:01:21 PDT
Created attachment 351324 [details]
Updated patchh
Comment 5 Eric Carlson 2018-10-01 23:41:53 PDT
Created attachment 351346 [details]
Updated patchh
Comment 6 youenn fablet 2018-10-02 01:18:18 PDT
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?
Comment 7 Eric Carlson 2018-10-02 02:28:49 PDT
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.
Comment 8 Eric Carlson 2018-10-02 03:04:15 PDT
Created attachment 351364 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-10-02 04:18:57 PDT
Comment on attachment 351364 [details]
Patch for landing

Clearing flags on attachment: 351364

Committed r236730: <https://trac.webkit.org/changeset/236730>
Comment 10 WebKit Commit Bot 2018-10-02 04:18:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Matt Lewis 2018-10-04 15:25:13 PDT
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>
Comment 12 Matt Lewis 2018-10-04 15:29:33 PDT
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
Comment 13 Matt Lewis 2018-10-04 15:31:35 PDT
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.
Comment 14 Eric Carlson 2018-10-05 09:53:15 PDT
Created attachment 351678 [details]
Fix test crash
Comment 15 WebKit Commit Bot 2018-10-05 10:20:29 PDT
Comment on attachment 351678 [details]
Fix test crash

Clearing flags on attachment: 351678

Committed r236877: <https://trac.webkit.org/changeset/236877>
Comment 16 WebKit Commit Bot 2018-10-05 10:20:30 PDT
All reviewed patches have been landed.  Closing bug.