Bug 165476 – [MediaStream][Mac] Revoke sandbox extensions when capture ends

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Revoke sandbox extensions issued to the WebProcess when all capture sessions end.

[Eric Carlson]

Created attachment 296311 [details]
Proposed patch.

[Brady Eidson]

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> ?

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 296317 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 296317 [details]
Patch for landing.

Clearing flags on attachment: 296317

Committed r209422: <http://trac.webkit.org/changeset/209422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r209422 for reason:

This change caused assertion failures during API tests.

Committed r209449: <http://trac.webkit.org/changeset/209449>

[Eric Carlson]

Created attachment 296423 [details]
Updated patch.

[Brady Eidson]

Comment on attachment 296423 [details]
Updated patch.

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

Get the bots green. :)

[Eric Carlson]

Created attachment 296433 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 296433 [details]
Patch for landing.

Clearing flags on attachment: 296433

Committed r209512: <http://trac.webkit.org/changeset/209512>

[Radar WebKit Bug Importer]

<rdar://problem/29993828>