Add sandbox ISA and extra compile flag fields to PNaCl translation cache key

Add sandbox ISA and extra compile flag fields to PNaCl translation cache key

Previously there was no indication to the translation cache of what
architecture the translated nexes were compiled for. This was not a
problem in a world where the architecture of chrome (or Windows) did
not change, but this will shortly happen on OSX, and we may support
multiple sandboxes in the future.

This adds 2 fields to the translation cache info, so that we can
differentiate the architectures and retrieve nexes that will actually
work. The field for extra compile flags is not being used yet, but
may be in the future if we start using machine-specific optimizations.

R=jvoung@chromium.org,teravest@chromium.org
BUG=316912
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251835

[Derek Schuff, 2014-02-15 01:45:39]

jvoung: review
teravest: ppapi OWNERS (however as this doesn't significantly affect the code of
the plugin/renderer and doesn't change any public APIs, you might possibly end
up getting TBRed)

[jvoung (off chromium), 2014-02-15 02:07:27]

otherwise LGTM

https://codereview.chromium.org/163433015/diff/1/components/nacl/browser/pnac...
File components/nacl/browser/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/163433015/diff/1/components/nacl/browser/pnac...
components/nacl/browser/pnacl_translation_cache_unittest.cc:192:
PnaclTranslationCache::GetKey(info));
maybe add a test where info.extra_flags is actually set to something?

https://codereview.chromium.org/163433015/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_host_messages.h (right):

https://codereview.chromium.org/163433015/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_host_messages.h:46:
IPC_STRUCT_TRAITS_MEMBER(sandbox_isa)
You could also get the nacl arch from "OmahaQueryParams::GetNaclArch()" on the
cache/browser side w/out consulting the trusted plugin.

However, that was mostly intended for native NaCl + webstore and maybe PNaCl
would end up with different set of sandboxes from native NaCl so it might want
be independent.

[jvoung (off chromium), 2014-02-15 02:08:44]

https://codereview.chromium.org/163433015/diff/1/ppapi/api/private/ppb_nacl_p...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/163433015/diff/1/ppapi/api/private/ppb_nacl_p...
ppapi/api/private/ppb_nacl_private.idl:116: * |abi_version|, |opt_level|,
|last_modified|, |etag|, and
update comment about cache information too

[Derek Schuff, 2014-02-15 05:59:18]

guess we need +jschuh as well for the new fields in the IPC

https://codereview.chromium.org/163433015/diff/1/components/nacl/browser/pnac...
File components/nacl/browser/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/163433015/diff/1/components/nacl/browser/pnac...
components/nacl/browser/pnacl_translation_cache_unittest.cc:192:
PnaclTranslationCache::GetKey(info));
On 2014/02/15 02:07:27, jvoung (cr) wrote:
> maybe add a test where info.extra_flags is actually set to something?

Done.

https://codereview.chromium.org/163433015/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_host_messages.h (right):

https://codereview.chromium.org/163433015/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_host_messages.h:46:
IPC_STRUCT_TRAITS_MEMBER(sandbox_isa)
On 2014/02/15 02:07:27, jvoung (cr) wrote:
> You could also get the nacl arch from "OmahaQueryParams::GetNaclArch()" on the
> cache/browser side w/out consulting the trusted plugin.
> 
> However, that was mostly intended for native NaCl + webstore and maybe PNaCl
> would end up with different set of sandboxes from native NaCl so it might want
> be independent.

Given that the rest of the cache information is coming from the plugin, and we
are adding the extra flag field anyway, I think it makes the most sense to keep
them all together for now. once we get rid of the plugin we can unify them.

edit: also it turns out that OmahaQueryParams is in Chrome and can't be depended
on by components, so it maybe should be moved or something, but that can be
another CL

https://codereview.chromium.org/163433015/diff/1/ppapi/api/private/ppb_nacl_p...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/163433015/diff/1/ppapi/api/private/ppb_nacl_p...
ppapi/api/private/ppb_nacl_private.idl:116: * |abi_version|, |opt_level|,
|last_modified|, |etag|, and
On 2014/02/15 02:08:44, jvoung (cr) wrote:
> update comment about cache information too

Done.

[jschuh, 2014-02-15 15:36:40]

On 2014/02/15 05:59:18, Derek Schuff wrote:
> guess we need +jschuh as well for the new fields in the IPC

Or, you know, one of the *many* other IPC reviewers. :)

https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
ppapi/api/private/ppb_nacl_private.idl:134: [in] str_t extra_flags,
Why are these strings rather than enums/bit-flags? Strings scare me, because
they imply parsing.

[teravest, 2014-02-18 15:23:06]

lgtm

[Derek Schuff, 2014-02-18 17:20:31]

On 2014/02/15 15:36:40, Justin Schuh wrote:
> On 2014/02/15 05:59:18, Derek Schuff wrote:
> > guess we need +jschuh as well for the new fields in the IPC
> 
> Or, you know, one of the *many* other IPC reviewers. :)

that's a fair point: is it better to get the same person who reviewed this code
when it went in (you) in case you still remember anything about it, or better to
spread the reviews around?

> 
>
https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
> File ppapi/api/private/ppb_nacl_private.idl (right):
> 
>
https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
> ppapi/api/private/ppb_nacl_private.idl:134: [in] str_t extra_flags,
> Why are these strings rather than enums/bit-flags? Strings scare me, because
> they imply parsing.

there's no parsing, they are just going directly into the cache key the same way
the URL and etag and other components of PNaClCacheInfo are now.

[jschuh, 2014-02-18 17:30:34]

On 2014/02/18 17:20:31, Derek Schuff wrote:
> On 2014/02/15 15:36:40, Justin Schuh wrote:
> > On 2014/02/15 05:59:18, Derek Schuff wrote:
> > > guess we need +jschuh as well for the new fields in the IPC
> > 
> > Or, you know, one of the *many* other IPC reviewers. :)
> 
> that's a fair point: is it better to get the same person who reviewed this
code
> when it went in (you) in case you still remember anything about it, or better
to
> spread the reviews around?

It is better for continuity to stick to one reviewer per feature. I'm just
ornery because I spent most of last week reviewing CLs and barely got to write a
line of code.


>
https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
> > File ppapi/api/private/ppb_nacl_private.idl (right):
> > 
> >
>
https://codereview.chromium.org/163433015/diff/110001/ppapi/api/private/ppb_n...
> > ppapi/api/private/ppb_nacl_private.idl:134: [in] str_t extra_flags,
> > Why are these strings rather than enums/bit-flags? Strings scare me, because
> > they imply parsing.
> 
> there's no parsing, they are just going directly into the cache key the same
way
> the URL and etag and other components of PNaClCacheInfo are now.

Okay, ipc security lgtm, but if anyone ever tries to parse these I will find you
and make you pay... in bitcoin!

[Derek Schuff, 2014-02-18 18:23:54]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-02-18 18:34:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/163433015/110001

[commit-bot: I haz the power, 2014-02-18 18:34:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-18 18:34:33]

Failed to apply patch for ppapi/c/private/ppb_nacl_private.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/c/private/ppb_nacl_private.h
  Hunk #1 FAILED at 3.
  Hunk #2 FAILED at 127.
  Hunk #3 succeeded at 150 (offset 2 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
ppapi/c/private/ppb_nacl_private.h.rej

Patch:       ppapi/c/private/ppb_nacl_private.h
Index: ppapi/c/private/ppb_nacl_private.h
diff --git a/ppapi/c/private/ppb_nacl_private.h
b/ppapi/c/private/ppb_nacl_private.h
index
0001148de9c3f033af12e7a4138d98a3026ac06c..ee632354a7b8ead12a9f18193a4a3bfdb7c000e0
100644
--- a/ppapi/c/private/ppb_nacl_private.h
+++ b/ppapi/c/private/ppb_nacl_private.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Mon Feb 10 11:05:29 2014. */
+/* From private/ppb_nacl_private.idl modified Fri Feb 14 21:39:55 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -127,19 +127,18 @@ struct PPB_NaCl_Private_1_0 {
    * returns a posix handle to that temporary file.
    */
   PP_FileHandle (*CreateTemporaryFile)(PP_Instance instance);
-  /* Create a temporary file, which will be deleted by the time the
-   * last handle is closed (or earlier on POSIX systems), to use for
-   * the nexe with the cache information given by |pexe_url|,
-   * |abi_version|, |opt_level|, |last_modified|, |etag|, and
-   * |has_no_store_header|. If the nexe is already present in the
-   * cache, |is_hit| is set to PP_TRUE and the contents of the nexe
-   * will be copied into the temporary file. Otherwise |is_hit| is set
-   * to PP_FALSE and the temporary file will be writeable.  Currently
-   * the implementation is a stub, which always sets is_hit to false
-   * and calls the implementation of CreateTemporaryFile. In a
-   * subsequent CL it will call into the browser which will remember
-   * the association between the cache key and the fd, and copy the
-   * nexe into the cache after the translation finishes.
+  /* Create a temporary file, which will be deleted by the time the last handle
+   * is closed (or earlier on POSIX systems), to use for the nexe with the
cache
+   * information given by |pexe_url|, |abi_version|, |opt_level|,
+   * |last_modified|, |etag|, |has_no_store_header|, |sandbox_isa|, and
+   * |extra_flags|. If the nexe is already present in the cache, |is_hit| is
set
+   * to PP_TRUE and the contents of the nexe will be copied into the temporary
+   * file. Otherwise |is_hit| is set to PP_FALSE and the temporary file will be
+   * writeable.  Currently the implementation is a stub, which always sets
+   * is_hit to false and calls the implementation of CreateTemporaryFile. In a
+   * subsequent CL it will call into the browser which will remember the
+   * association between the cache key and the fd, and copy the nexe into the
+   * cache after the translation finishes.
    */
   int32_t (*GetNexeFd)(PP_Instance instance,
                        const char* pexe_url,
@@ -148,6 +147,8 @@ struct PPB_NaCl_Private_1_0 {
                        const char* last_modified,
                        const char* etag,
                        PP_Bool has_no_store_header,
+                       const char* sandbox_isa,
+                       const char* extra_flags,
                        PP_Bool* is_hit,
                        PP_FileHandle* nexe_handle,
                        struct PP_CompletionCallback callback);

[Derek Schuff, 2014-02-18 18:49:04]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-02-18 18:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/163433015/370001

[commit-bot: I haz the power, 2014-02-18 20:59:55]

Change committed as 251835