Summary: | [MediaStream][Mac] Revoke sandbox extensions when capture ends | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dino, jer.noble, ryanhaddad, thiago.lacerda, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Eric Carlson
2016-12-06 10:22:39 PST
Created attachment 296311 [details]
Proposed patch.
Comment on attachment 296311 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=296311&action=review I think some RefPtr<>s can and should be Ref<>s instead. Otherwise lgtm > Source/WebKit2/UIProcess/WebPageProxy.cpp:6425 > + if (!oldMediaStateHasActiveCapture && newMediaStateHasActiveCapture) { > + m_uiClient->didBeginCaptureSession(); > + m_userMediaPermissionRequestManager.startedCaptureSession(); > + } > + if (oldMediaStateHasActiveCapture && !newMediaStateHasActiveCapture) { > + m_uiClient->didEndCaptureSession(); > + m_userMediaPermissionRequestManager.endedCaptureSession(); > + } nit: else if (...) > Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.cpp:59 > +std::pair<String, RefPtr<SandboxExtension>> MediaDeviceSandboxExtensions::operator[](size_t i) Ref<SandboxExtension> ? > Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.cpp:66 > +const std::pair<String, RefPtr<SandboxExtension>> MediaDeviceSandboxExtensions::operator[](size_t i) const Ref<SandboxExtension> ? > Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.h:46 > + std::pair<String, RefPtr<SandboxExtension>> operator[](size_t i); > + const std::pair<String, RefPtr<SandboxExtension>> operator[](size_t i) const; Ref<SandboxExtension> ? > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:74 > + HashMap<String, RefPtr<SandboxExtension>> m_userMediaDeviceSandboxExtensions; Ref<SandboxExtension> ? Comment on attachment 296311 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=296311&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:6425 >> + } > > nit: else if (...) Fixed. >> Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.cpp:59 >> +std::pair<String, RefPtr<SandboxExtension>> MediaDeviceSandboxExtensions::operator[](size_t i) > > Ref<SandboxExtension> ? That doesn't work because we need to call SandboxExtension.consume and revoke, but they are not const. >> Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.cpp:66 >> +const std::pair<String, RefPtr<SandboxExtension>> MediaDeviceSandboxExtensions::operator[](size_t i) const > > Ref<SandboxExtension> ? Ditto. >> Source/WebKit2/WebProcess/MediaStream/MediaDeviceSandboxExtensions.h:46 >> + const std::pair<String, RefPtr<SandboxExtension>> operator[](size_t i) const; > > Ref<SandboxExtension> ? Ditto. >> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:74 >> + HashMap<String, RefPtr<SandboxExtension>> m_userMediaDeviceSandboxExtensions; > > Ref<SandboxExtension> ? Ditto. Created attachment 296317 [details]
Patch for landing.
Comment on attachment 296317 [details] Patch for landing. Clearing flags on attachment: 296317 Committed r209422: <http://trac.webkit.org/changeset/209422> All reviewed patches have been landed. Closing bug. (In reply to comment #5) > Comment on attachment 296317 [details] > Patch for landing. > > Clearing flags on attachment: 296317 > > Committed r209422: <http://trac.webkit.org/changeset/209422> This change appears to have caused assertion failures for 8 API tests: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK1%20%28Tests%29/builds/10154 ASSERTION FAILED: stateMap().contains(&proxy.page().process()) /Volumes/Data/slave/elcapitan-debug/build/Source/WebKit2/UIProcess/UserMediaProcessManager.cpp(163) : void WebKit::UserMediaProcessManager::endedCaptureSession(WebKit::UserMediaPermissionRequestManagerProxy &) 1 0x105453d60 WTFCrash 2 0x107edf300 WebKit::UserMediaProcessManager::endedCaptureSession(WebKit::UserMediaPermissionRequestManagerProxy&) 3 0x107edf19e WebKit::UserMediaProcessManager::removeUserMediaPermissionRequestManagerProxy(WebKit::UserMediaPermissionRequestManagerProxy&) 4 0x108091c55 WebKit::UserMediaPermissionRequestManagerProxy::~UserMediaPermissionRequestManagerProxy() 5 0x108091ca5 WebKit::UserMediaPermissionRequestManagerProxy::~UserMediaPermissionRequestManagerProxy() 6 0x10835db7c WebKit::WebPageProxy::~WebPageProxy() 7 0x1083613a5 WebKit::WebPageProxy::~WebPageProxy() 8 0x10865e25d -[WKObject release] 9 0x107b8536d API::Object::deref() 10 0x10815b5e0 WTF::Ref<WebKit::WebPageProxy>::~Ref() 11 0x108159a95 WTF::Ref<WebKit::WebPageProxy>::~Ref() 12 0x1085db3b2 WebKit::WebViewImpl::~WebViewImpl() 13 0x1085db4d5 WebKit::WebViewImpl::~WebViewImpl() 14 0x1086bb236 -[WKView dealloc] 15 0x7fff95081ac4 (anonymous namespace)::AutoreleasePoolPage::pop(void*) 16 0x7fff8beaec12 _CFAutoreleasePoolPop 17 0x7fff8762f9ea -[NSAutoreleasePool drain] 18 0x103babf4c main 19 0x7fff8febf5ad start 20 0x2 Reverted r209422 for reason: This change caused assertion failures during API tests. Committed r209449: <http://trac.webkit.org/changeset/209449> Created attachment 296423 [details]
Updated patch.
Comment on attachment 296423 [details]
Updated patch.
I looked at the diff between the two patches. New one's fine.
Get the bots green. :)
Created attachment 296433 [details]
Patch for landing.
Comment on attachment 296433 [details] Patch for landing. Clearing flags on attachment: 296433 Committed r209512: <http://trac.webkit.org/changeset/209512> |