Open NaCl IRT file only once at startup

Open NaCl IRT file only once at startup

This changes the NaCl startup dance so that the IRT file is opened
just once at browser startup.  That file descriptor is kept around and
passed repeatedly to each NaCl process launched.  This ensures that
when autoupdate replaces the file on disk with a new version, we
continue to use the original file that corresponds to the old browser
version that's still running.

We also eliminate the cases for not having an IRT file, which is now a
hard error (i.e. prevents NaCl launches).  It's been a hard
requirement for NaCl that the IRT be available since Chromium 14.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=1772
TEST= hand-tested in Chromium build on Linux, Mac, and Windows

R=brettw@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110136

[Roland McGrath, 2011-10-27 17:04:14]

ping?

[brettw, 2011-10-28 18:54:31]

Sorry for the delay, I'm half on leave these days.

http://codereview.chromium.org/8397001/diff/1/chrome/browser/chrome_browser_m...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8397001/diff/1/chrome/browser/chrome_browser_m...
chrome/browser/chrome_browser_main.cc:1888:
NaClProcessHost::OpenIrtLibraryFile();
You're doing blocking I/O on the UI thread blocking the startup path, which is
basically the worst thing you can do in Chrome. We need to get this onto a
background thread and I'd also like to do it lazily (since most people won't be
using NaCl at all, it's wasteful to open it every time).

[Roland McGrath, 2011-10-28 19:27:13]

On 2011/10/28 18:54:31, brettw wrote:
> Sorry for the delay, I'm half on leave these days.

If there's a better reviewer, feel free to punt me to someone else.

>
http://codereview.chromium.org/8397001/diff/1/chrome/browser/chrome_browser_m...
> chrome/browser/chrome_browser_main.cc:1888:
> NaClProcessHost::OpenIrtLibraryFile();
> You're doing blocking I/O on the UI thread blocking the startup path, which is
> basically the worst thing you can do in Chrome. 

Ok.  Note this is just opening the file, not doing any actual data reads.

> We need to get this onto a background thread and I'd also like to do it

Can you give me any pointers to how to send this to an appropriate thread?

> lazily (since most people won't be using NaCl at all, it's wasteful to
> open it every time).

Doing it lazily defeats the fundamental purpose.  We need to have the file
opened when Chrome starts up so that we're sure it's still the same file
that came with this Chrome binary.  If you start Chrome, run for a while,
then autoupdate comes along and replaces all the files from under you,
then you start using NaCl in the same running browser, you lose.
That's what I'm fixing.


Thanks,
Roland

[brettw, 2011-10-28 19:55:52]

Adding erg for the handle issue. On Linux I guess we do keep some handles open
to avoid the autoupdate issue, but I don't know how this works or when this
happens. If we need to do something, we should do it on Linux only.

[Roland McGrath, 2011-10-28 20:00:39]

On 2011/10/28 19:55:52, brettw wrote:
> Adding erg for the handle issue. On Linux I guess we do keep some handles open
> to avoid the autoupdate issue, but I don't know how this works or when this
> happens. If we need to do something, we should do it on Linux only.

I was trying to avoid unnecessary divergence among platforms.  It is also a
performance benefit to reuse the open file descriptor rather than opening
it by name again every time.  But it's true that AIUI on non-Linux we could
open it only on the first use--assuming that the running version's
directory of files always remains on disk at least until the browser stops
running.


Thanks,
Roland

[Elliot Glaysher, 2011-10-28 20:05:23]

http://codereview.chromium.org/8397001/diff/1/chrome/browser/chrome_browser_m...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8397001/diff/1/chrome/browser/chrome_browser_m...
chrome/browser/chrome_browser_main.cc:1888:
NaClProcessHost::OpenIrtLibraryFile();
On 2011/10/28 18:54:32, brettw wrote:
> You're doing blocking I/O on the UI thread blocking the startup path, which is
> basically the worst thing you can do in Chrome. We need to get this onto a
> background thread

Agreed. To interact with the package manager correctly, we need this as right
after we start up chrome, but we don't need to block the critical startup path
here. It can be a low priority file thread thing since we don't use nacl on the
ntp or anything.

This should kick off an asynchronous load (and probably on linux only) and then
keep that fd around.

[brettw, 2011-10-28 20:11:31]

On 2011/10/28 20:00:39, Roland McGrath wrote:
> On 2011/10/28 19:55:52, brettw wrote:
> > Adding erg for the handle issue. On Linux I guess we do keep some handles
open
> > to avoid the autoupdate issue, but I don't know how this works or when this
> > happens. If we need to do something, we should do it on Linux only.
> 
> I was trying to avoid unnecessary divergence among platforms.  It is also a
> performance benefit to reuse the open file descriptor rather than opening
> it by name again every time.

It doesn't make sense to make browser startup slower for all users in order to
make NaCl plugin loading faster.

The versioned directory will hang around on non-Linux systems and you shouldn't
need to worry about it.

[Roland McGrath, 2011-10-28 21:20:32]

I'm going to rework this to be less synchronous.  Probably take a few days.

[Roland McGrath, 2011-10-31 20:19:09]

PTAL

[brettw, 2011-11-01 04:50:49]

Sorry, I'll be going on leave tomorrow, so I'm going to defer to Elliot.

I do wonder if the opening of the file should be delayed. It seems like we
should follow what we've done in other places (not entirely sure what this is.)

(LGTM to make commit-bot happy, but this isn't a review.)

[Roland McGrath, 2011-11-01 16:49:20]

On 2011/11/01 04:50:49, brettw wrote:
> Sorry, I'll be going on leave tomorrow, so I'm going to defer to Elliot.

Ok.  Elliot, PTAL.

> I do wonder if the opening of the file should be delayed. It seems like we
> should follow what we've done in other places (not entirely sure what this
is.)

Delayed until when?  On non-Linux (and on ChromeOS), it's already opened on
first use.  On regular Linux, it has to be opened "at startup" to fix the
autoupdate problem.  So I assume you're talking about doing it at some
slightly later stage of startup.

I placed the call to NaClProcessHost::EarlyStartup() based on a naive
reading of the startup path.  It looked to me like it's the last place that
initialization sort of stuff is done before actual page loads will start to
happen.


Thanks,
Roland

[Elliot Glaysher, 2011-11-01 18:24:17]

lgtm from a timing issue perspective

(If you could get another nacl person to look over this, that'd be awesome
though. I assume the fd code is correct, but I'm not an expert in that.)

[commit-bot: I haz the power, 2011-11-12 00:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcgrathr@chromium.org/8397001/15001

[commit-bot: I haz the power, 2011-11-12 00:19:58]

Presubmit check for 8397001-15001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/renderer_host/chrome_render_message_filter.cc:150

Presubmit checks took 2.2s to calculate.