[webkit-reviews] review granted: [Bug 205967] [Media in GPU process] Extend the GPU process sandbox to allow access to local files when necessary : [Attachment 387266] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 205967: [Media in GPU process] Extend the GPU process sandbox to allow
access to local files when necessary
https://bugs.webkit.org/show_bug.cgi?id=205967

Attachment 387266: Patch

https://bugs.webkit.org/attachment.cgi?id=387266&action=review




--- Comment #10 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 387266
  --> https://bugs.webkit.org/attachment.cgi?id=387266
Patch

LGTM.
I think we can improve the handling of
mediaCacheDirectory/mediaKeyStorageDirectory here or as follow-up.

View in context: https://bugs.webkit.org/attachment.cgi?id=387266&action=review

> Source/WebKit/GPUProcess/GPUProcess.h:87
> +    HashMap<URL, RefPtr<SandboxExtension>> m_mediaCaptureSandboxExtensions;

s/RefPtr/Ref/

> Source/WebKit/GPUProcess/GPUProcessMediaConfiguration.h:44
> +    String mediaKeyStorageDirectory;

I do not see mediaCacheDirectory and mediaKeyStorageDirectory being used,
probably because we are sending this information for each remote player.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:102
> +	       return;

Rethinking about it, we might not need to error the load here.
If the sandbox extension cannot be created, the load will ultimately fail,
there will be a sandbox violation reported in the console.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:131
> +    if (!m_haveExtendedProcessSandbox) {

We cannot really use a boolean since GPUProcess might support more than one
sessionID.
I guess for now, I would remove m_haveExtendedProcessSandbox and always send
this information.

As follow-up, it seems we should improve the handling of
mediaCacheDirectory/mediaKeyStorageDirectory:

We currently send mediaCacheDirectory/mediaKeyStorageDirectory to GPUProcess
when creating a remote player.
It would seem more consistent to send both sandbox extensions and directory
paths together.
We could send the sandboxes for each remote player but this creates some
overhead for each remote player.

I guess it might be best to store in GPUProcess a map SessionID -> MediaCache
configuration parameters.
GPUProcessProxy would send IPC messages to add entries in this map and remove
an entry when a WebsiteDataStore gets deleted.
When creating a remote player in GPU Process, we would query the map to get the
corresponding cache directory paths.

Also, if we are sending the mediaCacheDirectory/mediaKeyStorageDirectory
sandboxes from GPUProcessProxy to GPUProcess, we probably do not need to send
the same sandbox extensions to WebProcess in that configuration.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:102
> +	   Optional<audit_token_t> auditToken =
m_manager.gpuProcessConnection().auditToken();

s/Optional<audit_token_t>/auto/

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:340
> +    return *m_gpuProcessConnection;

Why not just "return WebProcess::singleton().ensureGPUProcessConnection();" and
remove m_gpuProcessConnection?

> Source/WebKit/WebProcess/WebProcess.cpp:1303
> +	  
m_gpuProcessConnection->setAuditToken(WTFMove(connectionInfo.auditToken));

Should we assert here that auditToken is not null?

> LayoutTests/gpu-process/TestExpectations:220
> +media/video-play-audio-require-user-gesture.html [ Pass ]

Nice!