Summary: | [MediaStream] RealtimeMediaSource should be able to vend hashed IDs | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||
Component: | WebRTC | Assignee: | 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
Eric Carlson
2018-10-01 09:49:27 PDT
Created attachment 351273 [details]
Patch
Created attachment 351292 [details]
Updated patch
Created attachment 351324 [details]
Updated patchh
Created attachment 351346 [details]
Updated patchh
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 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. Created attachment 351364 [details]
Patch for landing
Comment on attachment 351364 [details] Patch for landing Clearing flags on attachment: 351364 Committed r236730: <https://trac.webkit.org/changeset/236730> All reviewed patches have been landed. Closing bug. 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> 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 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. Created attachment 351678 [details]
Fix test crash
Comment on attachment 351678 [details] Fix test crash Clearing flags on attachment: 351678 Committed r236877: <https://trac.webkit.org/changeset/236877> All reviewed patches have been landed. Closing bug. |