Implement EmbeddedWorkerContextClient.setCachedMetadata/clearCachedMetadata

Implement EmbeddedWorkerContextClient.setCachedMetadata/clearCachedMetadata

This cl depends on
Chromium:
  - Introduce AppCacheResponseMetadataWriter to support metadata caching.
    https://codereview.chromium.org/932643002/
Blink:
  - CachedMetadata support for ServiceWorker script.
    https://codereview.chromium.org/929953002/

After https://codereview.chromium.org/929953002/, EmbeddedWorkerContextClient.setCachedMetadata/clearCachedMetadata will be called when V8ScriptRunner calls CachedMetadataHandler.setCachedMetadata/clearCachedMetadata in a ServiceWorker.
This cl implements these methods.

When these methods are called, ServiceWorkerScriptContext sends IPC to the broeser process.
ServiceWorkerVersion recieves these messages and calls ServiceWorkerScriptCacheMap.WriteMetadata/ClearMetadata.
ServiceWorkerScriptCacheMap creates ServiceWorkerResponseMetadataWriter to write/clear the metadata in ServiceWorkerStorage.

ServiceWorkerResponseMetadataWriter is a subclass of AppCacheResponseMetadataWriter which is introduced https://codereview.chromium.org/932643002/.

TEST=content_unittests.exe --gtest_filter=ServiceWorkerResourceStorageTest.*
BUG=449895
Committed: https://crrev.com/c514247c033d77fff1481a94fc923c907baa3fe5
Cr-Commit-Position: refs/heads/master@{#317441}

[horo, 2015-02-17 13:18:19]

horo@chromium.org changed reviewers:
+ michaeln@chromium.org

[horo, 2015-02-17 13:18:19]

michaeln@
Could you please review this?

Thank you!

[michaeln, 2015-02-18 03:37:34]

lgtm modulo nits

https://codereview.chromium.org/931173002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/931173002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_script_cache_map.h:80: int);
name the last arg

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:237: std::vector<char>
/* data */)
how large can the data be?

https://codereview.chromium.org/931173002/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/931173002/diff/1/content/renderer/service_wor...
content/renderer/service_worker/service_worker_script_context.h:80: void
clearCachedMetadata(const GURL& url);
Upper first letter in method names.

[horo, 2015-02-18 04:13:29]

New patchsets have been uploaded after l-g-t-m from michaeln@chromium.org

[horo, 2015-02-18 04:17:06]

Thank you!

https://codereview.chromium.org/931173002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/931173002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_script_cache_map.h:80: int);
On 2015/02/18 03:37:34, michaeln wrote:
> name the last arg

Done.

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:237: std::vector<char>
/* data */)
On 2015/02/18 03:37:34, michaeln wrote:
> how large can the data be?

Pickle::WriteBytes() treats the length as int.
So I think this can be 2GB.

https://codereview.chromium.org/931173002/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/931173002/diff/1/content/renderer/service_wor...
content/renderer/service_worker/service_worker_script_context.h:80: void
clearCachedMetadata(const GURL& url);
On 2015/02/18 03:37:34, michaeln wrote:
> Upper first letter in method names.

Done.

[horo, 2015-02-18 04:58:58]

horo@chromium.org changed reviewers:
+ wfh@chromium.org

[horo, 2015-02-18 04:58:58]

wfh@
Could you please review content/common/service_worker/service_worker_messages.h?

[Will Harris, 2015-02-18 06:00:34]

This concerns me, maybe I need more explanation but it looks like it allows any
renderer process to write to a specific ServiceWorkerResponseMetadataWriter
corresponding to a renderer control GURL?  Doesn't this mean a compromised
renderer can affect the cached data of another renderer?

[horo, 2015-02-18 06:29:34]

On 2015/02/18 06:00:34, Will Harris wrote:
> This concerns me, maybe I need more explanation but it looks like it allows
any
> renderer process to write to a specific ServiceWorkerResponseMetadataWriter
> corresponding to a renderer control GURL?  Doesn't this mean a compromised
> renderer can affect the cached data of another renderer?

No.
ServiceWorker has its own strage (ServiceWorkerStorage) for scripts.
The browser process handles Set/ClearCachedMetadata messages which are from the
ServiceWorker's renderer process.
If the message is sent from other compromised renderer process, the message will
not handled.

I refined the comment in service_worker_messages.h

[michaeln, 2015-02-18 19:17:00]

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:237: std::vector<char>
/* data */)
I guess I meant how large is this in practice. Processing very large messages
can be janky.

fyi, the ipc system puts a limit on the size of messages and the diskcache also
puts a limit on the size of resources.

ipc_channel.h
  // The maximum message size in bytes. Attempting to receive a message of this
  // size or bigger results in a channel error.
  static const size_t kMaximumMessageSize = 128 * 1024 * 1024;

[michaeln, 2015-02-18 19:22:01]

> On 2015/02/18 06:00:34, Will Harris wrote:
> > This concerns me, maybe I need more explanation but it looks like it allows
> any
> > renderer process to write to a specific ServiceWorkerResponseMetadataWriter
> > corresponding to a renderer control GURL?  Doesn't this mean a compromised
> > renderer can affect the cached data of another renderer?

This is only done for the main script resource so the url param actually isn't
needed, maybe we can remove that param.

Precomplied data is also written to the httpcache for normal pages (is that
right). What do the ipc messages to perform that function look like?

[horo, 2015-02-19 00:08:42]

horo@chromium.org changed reviewers:
+ vogelheim@chromium.org

[horo, 2015-02-19 00:08:43]

> This is only done for the main script resource so the url param actually isn't
> needed, maybe we can remove that param.

No we can't.
We also stores the V8 metadata cache of the scripts which are imported from the
main script using "importScripts()".
http://crrev.com/932773002


> Precomplied data is also written to the httpcache for normal pages (is that
> right). What do the ipc messages to perform that function look like?

Yes.
RendererBlinkPlatformImpl::cacheMetadata() sends
ViewHostMsg_DidGenerateCacheableMetadata message.

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/931173002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:237: std::vector<char>
/* data */)
On 2015/02/18 19:17:00, michaeln wrote:
> I guess I meant how large is this in practice. Processing very large messages
> can be janky.
> 
> fyi, the ipc system puts a limit on the size of messages and the diskcache
also
> puts a limit on the size of resources.
> 
> ipc_channel.h
>   // The maximum message size in bytes. Attempting to receive a message of
this
>   // size or bigger results in a channel error.
>   static const size_t kMaximumMessageSize = 128 * 1024 * 1024;


According to the UMA of V8.CodeCacheSizeRatio, the size of cached metadata seems
like less than 10x of the original code.
I think it is not very large.

[Tom Sepez, 2015-02-19 22:16:14]

On 2015/02/18 06:29:34, horo wrote:
> On 2015/02/18 06:00:34, Will Harris wrote:
> > This concerns me, maybe I need more explanation but it looks like it allows
> any
> > renderer process to write to a specific ServiceWorkerResponseMetadataWriter
> > corresponding to a renderer control GURL?  Doesn't this mean a compromised
> > renderer can affect the cached data of another renderer?
> 
> No.
> ServiceWorker has its own strage (ServiceWorkerStorage) for scripts.
> The browser process handles Set/ClearCachedMetadata messages which are from
the
> ServiceWorker's renderer process.
> If the message is sent from other compromised renderer process, the message
will
> not handled.
> 
> I refined the comment in service_worker_messages.h

What about a compromised service worker for site A attempting to cache resources
for site B's URL (service worksers are running scripts; it's possible they might
get owned).

[horo, 2015-02-19 23:07:38]

On 2015/02/19 22:16:14, Tom Sepez wrote:
> On 2015/02/18 06:29:34, horo wrote:
> > On 2015/02/18 06:00:34, Will Harris wrote:
> > > This concerns me, maybe I need more explanation but it looks like it
allows
> > any
> > > renderer process to write to a specific
ServiceWorkerResponseMetadataWriter
> > > corresponding to a renderer control GURL?  Doesn't this mean a compromised
> > > renderer can affect the cached data of another renderer?
> > 
> > No.
> > ServiceWorker has its own strage (ServiceWorkerStorage) for scripts.
> > The browser process handles Set/ClearCachedMetadata messages which are from
> the
> > ServiceWorker's renderer process.
> > If the message is sent from other compromised renderer process, the message
> will
> > not handled.
> > 
> > I refined the comment in service_worker_messages.h
> 
> What about a compromised service worker for site A attempting to cache
resources
> for site B's URL (service worksers are running scripts; it's possible they
might
> get owned).

The service worker for site A can't access to the scripts in
ServiceWorkerStorage
that are stored fot the service worker for site B.

More preceisely,
- Site X can have multiple ServiceWorkerRegistrations.
 The key of ServiceWorkerRegistration is a scope url.
- A ServiceWorkerRegistrations can have multiple ServiceWorkerVersions.
 If the script in the server is changed, a new ServiceWorkerVersion will be
 created.
- A ServiceWorkerVersion has its own ServiceWorkerScriptCacheMap.
 ServiceWorkerScriptCacheMap maintains the mapping between urls and a resource
 id for a particular version's implicit script resources.
 So ServiceWorkerVersion can't access to other ServiceWorkerVersion's script.

[Will Harris, 2015-02-19 23:55:14]

On 2015/02/19 23:07:38, horo wrote:

> The service worker for site A can't access to the scripts in
> ServiceWorkerStorage
> that are stored fot the service worker for site B.
> 
> More preceisely,
> - Site X can have multiple ServiceWorkerRegistrations.
>  The key of ServiceWorkerRegistration is a scope url.
> - A ServiceWorkerRegistrations can have multiple ServiceWorkerVersions.
>  If the script in the server is changed, a new ServiceWorkerVersion will be
>  created.
> - A ServiceWorkerVersion has its own ServiceWorkerScriptCacheMap.
>  ServiceWorkerScriptCacheMap maintains the mapping between urls and a resource
>  id for a particular version's implicit script resources.
>  So ServiceWorkerVersion can't access to other ServiceWorkerVersion's script.

Okay I understand that this what we want to happen, but I was more trying to be
pointed at the code where these checks were actually taking place.  I had to dig
a bit, but I found that this seems this is happening inside
EmbeddedWorkerRegistry::OnMessageReceived where the routing_id is mapped to a
unique EmbeddedWorkerInstance and then the message routed via
EmbeddedWorkerInstance::OnMessageReceived to all listeners.

Can you confirm I have this right?

[horo, 2015-02-20 00:37:59]

On 2015/02/19 23:55:14, Will Harris wrote:
> On 2015/02/19 23:07:38, horo wrote:
> 
> > The service worker for site A can't access to the scripts in
> > ServiceWorkerStorage
> > that are stored fot the service worker for site B.
> > 
> > More preceisely,
> > - Site X can have multiple ServiceWorkerRegistrations.
> >  The key of ServiceWorkerRegistration is a scope url.
> > - A ServiceWorkerRegistrations can have multiple ServiceWorkerVersions.
> >  If the script in the server is changed, a new ServiceWorkerVersion will be
> >  created.
> > - A ServiceWorkerVersion has its own ServiceWorkerScriptCacheMap.
> >  ServiceWorkerScriptCacheMap maintains the mapping between urls and a
resource
> >  id for a particular version's implicit script resources.
> >  So ServiceWorkerVersion can't access to other ServiceWorkerVersion's
script.
> 
> Okay I understand that this what we want to happen, but I was more trying to
be
> pointed at the code where these checks were actually taking place.  I had to
dig
> a bit, but I found that this seems this is happening inside
> EmbeddedWorkerRegistry::OnMessageReceived where the routing_id is mapped to a
> unique EmbeddedWorkerInstance and then the message routed via
> EmbeddedWorkerInstance::OnMessageReceived to all listeners.
> 
> Can you confirm I have this right?

Ah! Yes.
EmbeddedWorkerRegistry doesn't check the process id.
I uploaded patch set 5 which checks process_id in
EmbeddedWorkerRegistry::OnMessageReceived().

Thank you for pointing out!

[Will Harris, 2015-02-20 22:46:17]

On 2015/02/20 00:37:59, horo wrote:
> Ah! Yes.
> EmbeddedWorkerRegistry doesn't check the process id.
> I uploaded patch set 5 which checks process_id in
> EmbeddedWorkerRegistry::OnMessageReceived().
> 
> Thank you for pointing out!

okay great, thanks.

content/common/service_worker/service_worker_messages.h lgtm

[horo, 2015-02-20 22:47:30]

On 2015/02/20 22:46:17, Will Harris wrote:
> On 2015/02/20 00:37:59, horo wrote:
> > Ah! Yes.
> > EmbeddedWorkerRegistry doesn't check the process id.
> > I uploaded patch set 5 which checks process_id in
> > EmbeddedWorkerRegistry::OnMessageReceived().
> > 
> > Thank you for pointing out!
> 
> okay great, thanks.
> 
> content/common/service_worker/service_worker_messages.h lgtm

Thank you!

[horo, 2015-02-20 22:47:36]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-02-20 22:48:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/931173002/100001

[commit-bot: I haz the power, 2015-02-20 23:40:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-20 23:42:45]

Patchset 6 (id:??) landed as
https://crrev.com/c514247c033d77fff1481a94fc923c907baa3fe5
Cr-Commit-Position: refs/heads/master@{#317441}