Issue 1092005

Hi Bennett, 

this change is a work in progress.   Don't nitpick my code yet, but I've inserted several FIXME comments where I don't understand the existing code.  Your feedback would be much appreciated.

cheers
alan

- When fetching the NaCl module, include the architecture name in the URL.  Attempt
  to fetch (optionally) the architecture-specific URL, the PNaCl URL and the 
  architecture-independent URL (though this latter name should be considered deprecated).
- Use MIME type "application/x-nacl-srpc" throughout, eliminating x-nacl-npapi and
  x-nacl-npapi-over-srpc.
- Rename embed.src atrribute to embed.nexe to avoid eager fetching by browser, since
  the attribute no longer directly names a resource, but is mangled slightly.
- Fix all tests' HTML to use the new attribute name.  While we're there, add <noembed>
  elements to illustrate best practises.

Questions:
- how much architecture detail should we include in the resource name?
  "arm" is inadequate since there are several architectures; "armv7" (e.g.) would
  be better.  But this raises the question: what about neon?  SSE?  etc.

[bsy, 2010-03-23 21:20:37]

feedback on the fixmes, plus other kibbitzing.

http://codereview.chromium.org/1092005/diff/1/2
File src/trusted/plugin/srpc/plugin.cc (right):

http://codereview.chromium.org/1092005/diff/1/2#newcode246
src/trusted/plugin/srpc/plugin.cc:246: static const enum { BEFORE_ARCH,
AFTER_ARCH, NEVER } kTryPNaCl = BEFORE_ARCH;
might want ALWAYS/ONLY, for those people who prefer to ensure that the web stay
platform neutral. :-)

http://codereview.chromium.org/1092005/diff/1/2#newcode274
src/trusted/plugin/srpc/plugin.cc:274: "-x86-64.nexe"
not sure i like '-' as separator, esp since tools that parse filenames will have
to understand that in this case, it's the 2nd '-' that is the actual separator.

i actually prefer the subdirectory approach, since in the limit, data files may
also be architecture dependent, and this turns into a general PATH-like search
mechanism.  ditto for dynamic library loading -- the app may want to load a
dynamic library, but should the user be able to specify whether the pnacl
version is tried first?  last?  etc.

http://codereview.chromium.org/1092005/diff/1/2#newcode313
src/trusted/plugin/srpc/plugin.cc:313: // getting called?
i think StartDownload always fails a download by calling URLNotify, since
originally this was single threaded and all that's done is to put something on a
work queue -- possibly having initiated the DNS lookup for the URL via the dns
helper process.  not sure what the actual reasons why StartDownload would fail
here are.

though perhaps the oom case probably should result in the on fail handlers being
called.

http://codereview.chromium.org/1092005/diff/1/7
File src/trusted/plugin/srpc/srpc.cc (right):

http://codereview.chromium.org/1092005/diff/1/7#newcode442
src/trusted/plugin/srpc/srpc.cc:442: // non-NULL, and the other branch of the
if-statement is taken.
plz verify w/ sehr, but i think this distinction is whether the load was
initiated due to the <EMBED> tag or via NPN_URLNotify, where the plugin is
asking the browser to load something.  in the latter case, there is no on
failure handler, since the controlling code is C/C++, not javascript.

[sehr (please use chromium), 2010-03-23 21:37:16]

Confirming Bennet's assessment.

http://codereview.chromium.org/1092005/diff/1/7
File src/trusted/plugin/srpc/srpc.cc (right):

http://codereview.chromium.org/1092005/diff/1/7#newcode442
src/trusted/plugin/srpc/srpc.cc:442: // non-NULL, and the other branch of the
if-statement is taken.
On 2010/03/23 21:20:37, bsy wrote:
> plz verify w/ sehr, but i think this distinction is whether the load was
> initiated due to the <EMBED> tag or via NPN_URLNotify, where the plugin is
> asking the browser to load something.  in the latter case, there is no on
> failure handler, since the controlling code is C/C++, not javascript.

Bennet got this right.  We use the same code to download other assets (in the
SRPC-only world) that we use to download module.

[adonovan, 2010-03-25 00:44:05]

http://codereview.chromium.org/1092005/diff/1/2
File src/trusted/plugin/srpc/plugin.cc (right):

http://codereview.chromium.org/1092005/diff/1/2#newcode313
src/trusted/plugin/srpc/plugin.cc:313: // getting called?
On 2010/03/23 21:20:37, bsy wrote:
> i think StartDownload always fails a download by calling URLNotify, since
> originally this was single threaded and all that's done is to put something on
a
> work queue -- possibly having initiated the DNS lookup for the URL via the dns
> helper process.  not sure what the actual reasons why StartDownload would fail
> here are.

You're right, the notification always occurs, even on failure.

> though perhaps the oom case probably should result in the on fail handlers
being
> called.

Perhaps, but it doesn't seem particularly bad.

http://codereview.chromium.org/1092005/diff/1/7
File src/trusted/plugin/srpc/srpc.cc (right):

http://codereview.chromium.org/1092005/diff/1/7#newcode442
src/trusted/plugin/srpc/srpc.cc:442: // non-NULL, and the other branch of the
if-statement is taken.
On 2010/03/23 21:37:16, sehr wrote:
> On 2010/03/23 21:20:37, bsy wrote:
> > plz verify w/ sehr, but i think this distinction is whether the load was
> > initiated due to the <EMBED> tag or via NPN_URLNotify, where the plugin is
> > asking the browser to load something.  in the latter case, there is no on
> > failure handler, since the controlling code is C/C++, not javascript.
> 
> Bennet got this right.  We use the same code to download other assets (in the
> SRPC-only world) that we use to download module.


Regarding the onfail handler, I can't actually find any respectable
specifications of this HTML feature, so I guess it can't be that important.  In
any case, this is rather subtle and should probably be documented.


But regarding the data flow in this function, I still think it is unsound. 
Consider this: you cannot predict "reason"; it is determined by events outside
the control of the plugin, so its value is arbitrary, and not correlated with
the predicate "closure != NULL".  So, you don't learn the value of this
predicate based on the conditional control flow.  Yet the two arms of the
conditional make incompatible assumptions, the first that it is always true, the
second that it may be false.

If the first is right, then the onfail handler is unreachable.  If the second is
right, then we're possibly dereferencing a null pointer.

[sehr (please use chromium), 2010-04-07 03:57:38]

More overdue comments.

http://codereview.chromium.org/1092005/diff/1/7
File src/trusted/plugin/srpc/srpc.cc (right):

http://codereview.chromium.org/1092005/diff/1/7#newcode420
src/trusted/plugin/srpc/srpc.cc:420: // branch condition is not under our
control.
I fear this code has changed somewhat due Bennet's recent change.  I don't think
it's substantially different, though.

NPP_URLNotify is invoked in (to the best of my knowledge) exactly three
situations:
1) When a nacl module load is requested due to src= being changed.
2) When a resource is requested by either __UrlAsNaClDesc or NPN_GetURL{Notify}.
3) When a nacl module load happens implicitly because src= was set in the embed
tag or data= in the object tag.

(1) and (2) have non-null closures explicitly.
(3) is potentially at issue.  I like your change because it fuses (3) into (1).

http://codereview.chromium.org/1092005/diff/1/7#newcode442
src/trusted/plugin/srpc/srpc.cc:442: // non-NULL, and the other branch of the
if-statement is taken.
On 2010/03/25 00:44:05, adonovan wrote:
> On 2010/03/23 21:37:16, sehr wrote:
> > On 2010/03/23 21:20:37, bsy wrote:
> > > plz verify w/ sehr, but i think this distinction is whether the load was
> > > initiated due to the <EMBED> tag or via NPN_URLNotify, where the plugin is
> > > asking the browser to load something.  in the latter case, there is no on
> > > failure handler, since the controlling code is C/C++, not javascript.
> > 
> > Bennet got this right.  We use the same code to download other assets (in
the
> > SRPC-only world) that we use to download module.
> 
> 
> Regarding the onfail handler, I can't actually find any respectable
> specifications of this HTML feature, so I guess it can't be that important. 
In
> any case, this is rather subtle and should probably be documented.
> 
> 
> But regarding the data flow in this function, I still think it is unsound. 
> Consider this: you cannot predict "reason"; it is determined by events outside
> the control of the plugin, so its value is arbitrary, and not correlated with
> the predicate "closure != NULL".  So, you don't learn the value of this
> predicate based on the conditional control flow.  Yet the two arms of the
> conditional make incompatible assumptions, the first that it is always true,
the
> second that it may be false.
> 
> If the first is right, then the onfail handler is unreachable.  If the second
is
> right, then we're possibly dereferencing a null pointer.
> 
> 

I think this block is because of (3) above.  But you're right that either up
there or down here has a problem.