RFC: Add an interface for having the browser open a pnacl support file

RFC: Add an interface for having the browser open a pnacl support file (readonly) from the installed pnacl component directory.


   int (*GetReadonlyPnaclFd)(const char* filename);


Trajectory: Currently for pnacl dogfooding we use a chrome extension to store pnacl files and do a URL request to read them. This is nice because:
- we don't need any new interface to get to those files.
- the chrome installer isn't bloated.

However, we eventually want to use component updater instead of an extension, but that would require this new interface (or something like it).  All this is to decouple our install/updates from the chrome install because the pnacl files are currently large.


Why move to component updater:

- Component updater allows us to send more user-machine information to Omaha to pick the right .nexe architecture for the pnacl tools rather than download one large extension with all architectures.  This focuses the install/updates to 1/3 the files that were in the large extension (x86-32, x86-64, arm).
  - This is especially useful for Windows where there is a single version of chrome, so for things like the NaCl IRT we include both x86-32 and 64, just in case.  With the right info sent to Omaha, PNaCl can pick exactly the required x86-32 vs 64 components rather than just including both blindly.

- Similar to the above, we can test for the chrome revision and send out different packages to canary / beta / stable channels if needed.

- Installs can be profile-independent -- no separate copies per profile. It is installed in the profile directory like PepperFlash, but it is profile/Pnacl not profile/User1/Pnacl and profile/User2/Pnacl.

- Install via component updater will eventually require less user intervention (no going to the webstore). For now, this isn't quite true because the use of pnacl+component updater  is still hidden behind a flag because of size issues. Alternatively, there are "component extensions", which have a similar transparent install?

- URL requests in the NaCl plugin currently use stream-as-file, which makes copies of files into the temp directory. This avoids that copy. (30 - 200ms in a release build on Linux to do a URL load + copy, vs 0.4ms to just get the FD).


- See component updater design doc for more: https://docs.google.com/a/google.com/document/d/1gqKdDaoH9s8pbQkb9I_EX8Cfdsjg1QFEIZxtbH0Mqs4/edit?hl=en_US


Even without component updater, if we choose to bundle PNaCl with Chrome in ways other than an extension and use URL requests, we will need a similar interface.



BUG= http://code.google.com/p/nativeclient/issues/detail?id=2365
TEST= to be written...

[jvoung - send to chromium..., 2012-01-11 18:40:34]

updated the description un bit

[sehr (please use chromium), 2012-01-12 22:35:56]

On 2012/01/11 18:40:34, jvoung wrote:
> updated the description un bit

Description LGTM.  I think we should get Darin's opinion on it now, as brettw is
OOO.

[jvoung - send to chromium..., 2012-02-06 21:22:02]

+cc darin, brett

You can ignore the patch (I was just testing things), but... does the interface
and trajectory in CL description make sense? 

Thoughts very welcome -- thanks!

[brettw, 2012-02-06 21:59:06]

On Mon, Feb 6, 2012 at 1:22 PM,  <jvoung@google.com> wrote:
> +cc darin, brett
>
> You can ignore the patch (I was just testing things), but... does the
> interface
> and trajectory in CL description make sense?
>
> Thoughts very welcome -- thanks!

The general idea sounds fine to me. I didn't look at the details.

Brett

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[brettw, 2012-02-06 21:59:08]

On Mon, Feb 6, 2012 at 1:22 PM,  <jvoung@google.com> wrote:
> +cc darin, brett
>
> You can ignore the patch (I was just testing things), but... does the
> interface
> and trajectory in CL description make sense?
>
> Thoughts very welcome -- thanks!

The general idea sounds fine to me. I didn't look at the details.

Brett

[sehr (please use chromium), 2012-02-07 18:38:42]

On 2012/02/06 21:59:08, brettw wrote:
> On Mon, Feb 6, 2012 at 1:22 PM,  <mailto:jvoung@google.com> wrote:
> > +cc darin, brett
> >
> > You can ignore the patch (I was just testing things), but... does the
> > interface
> > and trajectory in CL description make sense?
> >
> > Thoughts very welcome -- thanks!
> 
> The general idea sounds fine to me. I didn't look at the details.
> 
> Brett

Brett,

Thanks.  Is there someone else Jan should have do the detailed review, then?

David

[brettw, 2012-02-07 19:32:10]

On Tue, Feb 7, 2012 at 10:38 AM,  <sehr@google.com> wrote:
> On 2012/02/06 21:59:08, brettw wrote:
>
>> On Mon, Feb 6, 2012 at 1:22 PM,  <mailto:jvoung@google.com> wrote:
>> > +cc darin, brett
>> >
>> > You can ignore the patch (I was just testing things), but... does the
>> > interface
>> > and trajectory in CL description make sense?
>> >
>> > Thoughts very welcome -- thanks!
>
>
>> The general idea sounds fine to me. I didn't look at the details.
>
>
>> Brett
>
>
> Brett,
>
> Thanks.  Is there someone else Jan should have do the detailed review, then?

No, but he said "You can ignore the patch (I was just testing things)"
so I didn't look at the details.

Brett

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[brettw, 2012-02-07 19:32:12]

On Tue, Feb 7, 2012 at 10:38 AM,  <sehr@google.com> wrote:
> On 2012/02/06 21:59:08, brettw wrote:
>
>> On Mon, Feb 6, 2012 at 1:22 PM,  <mailto:jvoung@google.com> wrote:
>> > +cc darin, brett
>> >
>> > You can ignore the patch (I was just testing things), but... does the
>> > interface
>> > and trajectory in CL description make sense?
>> >
>> > Thoughts very welcome -- thanks!
>
>
>> The general idea sounds fine to me. I didn't look at the details.
>
>
>> Brett
>
>
> Brett,
>
> Thanks.  Is there someone else Jan should have do the detailed review, then?

No, but he said "You can ignore the patch (I was just testing things)"
so I didn't look at the details.

Brett

[Mark Seaborn, 2012-02-26 21:02:44]

http://codereview.chromium.org/9158005/diff/31001/chrome/browser/nacl_host/pn...
File chrome/browser/nacl_host/pnacl_file_host.cc (right):

http://codereview.chromium.org/9158005/diff/31001/chrome/browser/nacl_host/pn...
chrome/browser/nacl_host/pnacl_file_host.cc:128: file_to_open =
base::CreatePlatformFile(filepath,

I would expect that won't work properly with autoupdate on Linux.  If
Chrome/Chromium has been upgraded but not yet restarted, you will start opening
new files that don't match with the currently-running version of Chromium.

See Evan Martin's blog post on the topic:
http://neugierig.org/software/chromium/notes/2011/08/zygote.html

We had to solve this problem with the IRT in the past.  See nacl_proces_host.cc
and the comment about autoupdate there.  (I don't know how the Component Updater
you referenced would change this situation though.)

Stepping back a bit, I suspect that whatever we do for the PNaCl translator's
files should match up with how we handle the IRT library.

How about this:

 * When a renderer process launches a NaCl process, it passes a list of names of
files (e.g. "irt_x86_32.nexe", "translator_x86_32.nexe", "crt1.o") to grant to
the NaCl process.

 * On Linux, the browser can pre-open all these files at startup.  I'm expecting
the number of files to be small enough for this to be OK, e.g. <=10 files.  If
it were 100s, we'd have to rethink this.  On other OSes, the browser can open
the requested files when the NaCl process is launched.

 * This avoids some IPC round trips for opening translator.nexe etc.

Mark

[brettw, 2012-02-26 21:49:59]

There is already some code to open the IRT a little after startup on Linux from
a background thread (it shouldn't actually block startup). I forget where that
is, but we should re-use that same code for these files as well.

[Use chromium.org instead, 2012-02-27 17:32:08]

On Sun, Feb 26, 2012 at 1:50 PM,  <brettw@chromium.org> wrote:
> There is already some code to open the IRT a little after startup on Linux
> from
> a background thread (it shouldn't actually block startup). I forget where
> that
> is, but we should re-use that same code for these files as well.

NaClProcessHost::EarlyStartup in chrome/browser/nacl_host/nacl_process_host.cc
does the startup call.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Use chromium.org instead, 2012-02-27 17:32:10]

On Sun, Feb 26, 2012 at 1:50 PM,  <brettw@chromium.org> wrote:
> There is already some code to open the IRT a little after startup on Linux
> from
> a background thread (it shouldn't actually block startup). I forget where
> that
> is, but we should re-use that same code for these files as well.

NaClProcessHost::EarlyStartup in chrome/browser/nacl_host/nacl_process_host.cc
does the startup call.

[jvoung - send to chromium..., 2012-06-19 18:29:41]

Thanks for the earlier comments Mark, and sorry about the delay -- sidetracked
by other work.  I'm thinking of looking at this again.

Earlier, I wasn't sure about whether to stick with component updater, or
consider alternatives to installing pnacl, given the size of the pnacl binaries.
 Given that Swiftshader also uses component updater, is also LLVM based, and is
smaller than the current pnacl binaries, I think I will stick with component
updater + some interface like this.

Comments inline.


On 2012/02/26 21:02:44, Mark Seaborn wrote:
>
http://codereview.chromium.org/9158005/diff/31001/chrome/browser/nacl_host/pn...
> File chrome/browser/nacl_host/pnacl_file_host.cc (right):
> 
>
http://codereview.chromium.org/9158005/diff/31001/chrome/browser/nacl_host/pn...
> chrome/browser/nacl_host/pnacl_file_host.cc:128: file_to_open =
> base::CreatePlatformFile(filepath,
> 
> I would expect that won't work properly with autoupdate on Linux.  If
> Chrome/Chromium has been upgraded but not yet restarted, you will start
opening
> new files that don't match with the currently-running version of Chromium.
> 
> See Evan Martin's blog post on the topic:
> http://neugierig.org/software/chromium/notes/2011/08/zygote.html
> 
> We had to solve this problem with the IRT in the past.  See
nacl_proces_host.cc
> and the comment about autoupdate there.  (I don't know how the Component
Updater
> you referenced would change this situation though.)


If we installed pnacl like the IRT, yes this would be a problem.

However, with component updater, I don't think this is a problem.  We control
the update + binary-publishing scheme.  So, even on linux we could download the
update to directory "REV+1" and wait till chrome restarts before publishing the
fact that we have "REV+1", and continue to use "REV" instead.  When we restart
chrome, we can clean up the old REV and publish REV+1 as the one to use.



> Stepping back a bit, I suspect that whatever we do for the PNaCl translator's
> files should match up with how we handle the IRT library.
> 
> How about this:
> 
>  * When a renderer process launches a NaCl process, it passes a list of names
of
> files (e.g. "irt_x86_32.nexe", "translator_x86_32.nexe", "crt1.o") to grant to
> the NaCl process.
> 
>  * On Linux, the browser can pre-open all these files at startup.  I'm
expecting
> the number of files to be small enough for this to be OK, e.g. <=10 files.  If
> it were 100s, we'd have to rethink this.  On other OSes, the browser can open
> the requested files when the NaCl process is launched.


It won't be hundreds of files right now -- more like 10 files.

However, as noted above, we don't need to pre-open files at startup.

The other thing is that we've tried to keep flags, lists of files out, etc., out
of the browser, and tried to contain that information in the translator itself,
so that translation wouldn't depend on the version of the browser as much.

This list of files can vary depending on whether you are generating a .nexe or a
.so, etc., so is chosen dynamically.  The files are currently requested via a
combination of:

- For nexes to start up (llc and ld), the plugin's file downloader (which we
will replace with "open") supplies this, and the startup code is then similar to
other nexes.

- For linker files, depending on .nexe or .so, certain sets of files are opened
via Bennet's reverse service interface, which bounces back to the plugin to do a
file download (which we will replace with "open").

Given this existing structure, would be worth it to changing the flow, or should
we just replace the file download with a file open, and take the hit of the IPCs
for now?


>  * This avoids some IPC round trips for opening translator.nexe etc.
> 
> Mark

[Mark Seaborn, 2012-06-20 20:12:55]

http://codereview.chromium.org/9158005/diff/34001/ppapi/native_client/src/tru...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/9158005/diff/34001/ppapi/native_client/src/tru...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:635: int fd =
get_readonly_pnacl_fd(filename.c_str());
I don't think get_readonly_pnacl_fd needs to be a global variable.  You can just
call GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) here.