Bug 165476

Summary: [MediaStream][Mac] Revoke sandbox extensions when capture ends
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: 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 Flags
Proposed patch.
beidson: review+
Patch for landing.
none
Updated patch.
beidson: review+
Patch for landing. none

Description Eric Carlson 2016-12-06 10:22:39 PST
Revoke sandbox extensions issued to the WebProcess when all capture sessions end.
Comment 1 Eric Carlson 2016-12-06 13:10:24 PST
Created attachment 296311 [details]
Proposed patch.
Comment 2 Brady Eidson 2016-12-06 13:31:51 PST
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 3 Eric Carlson 2016-12-06 14:04:38 PST
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.
Comment 4 Eric Carlson 2016-12-06 14:06:39 PST
Created attachment 296317 [details]
Patch for landing.
Comment 5 WebKit Commit Bot 2016-12-06 14:33:11 PST
Comment on attachment 296317 [details]
Patch for landing.

Clearing flags on attachment: 296317

Committed r209422: <http://trac.webkit.org/changeset/209422>
Comment 6 WebKit Commit Bot 2016-12-06 14:33:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryan Haddad 2016-12-06 17:25:47 PST
(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
Comment 8 Ryan Haddad 2016-12-06 21:32:27 PST
Reverted r209422 for reason:

This change caused assertion failures during API tests.

Committed r209449: <http://trac.webkit.org/changeset/209449>
Comment 9 Eric Carlson 2016-12-07 14:24:49 PST
Created attachment 296423 [details]
Updated patch.
Comment 10 Brady Eidson 2016-12-07 15:49:50 PST
Comment on attachment 296423 [details]
Updated patch.

I looked at the diff between the two patches. New one's fine.

Get the bots green. :)
Comment 11 Eric Carlson 2016-12-07 16:10:03 PST
Created attachment 296433 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2016-12-07 18:26:26 PST
Comment on attachment 296433 [details]
Patch for landing.

Clearing flags on attachment: 296433

Committed r209512: <http://trac.webkit.org/changeset/209512>
Comment 13 Radar WebKit Bug Importer 2017-01-12 10:40:04 PST
<rdar://problem/29993828>