Refine PNaCl pexe fetch UMA errors to know if the user aborted, etc.

Refine PNaCl pexe fetch UMA errors to know if the user aborted, etc.

Also check if it's access denied (e.g,, cross origin requests).

These additions mean that we are shifting the UMA codes
for PNaCl around.  This is okay since we haven't collected
any user samples yet.


BUG= http://code.google.com/p/nativeclient/issues/detail?id=3136


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177220

[jvoung (off chromium), 2013-01-15 20:35:48]

should have done the abort case earlier... sorry for the churn

https://codereview.chromium.org/11881038/diff/3001/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (left):

https://codereview.chromium.org/11881038/diff/3001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:706: //
TODO(dschuff): need to use url_util_->ResolveRelativeToURL?
It looks like the pexe URL is obtained from the manifest (like the nexe url),
which does the ResolveRelativeToURL for you.

[Derek Schuff, 2013-01-15 22:33:18]

Code looks pretty much ok. I guess we'll have to think about how to get the
error codes from llc back to the plugin... right now it's just the error string,
but we'll probably want to have the nexe output go to the JS console or
somewhere else rather than stderr anyway.

On 2013/01/15 20:35:48, jvoung (cr) wrote:
> should have done the abort case earlier... sorry for the churn
> 
>
https://codereview.chromium.org/11881038/diff/3001/ppapi/native_client/src/tr...
> File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (left):
> 
>
https://codereview.chromium.org/11881038/diff/3001/ppapi/native_client/src/tr...
> ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:706: //
> TODO(dschuff): need to use url_util_->ResolveRelativeToURL?
> It looks like the pexe URL is obtained from the manifest (like the nexe url),
> which does the ResolveRelativeToURL for you.

[Nick Bray (chromium), 2013-01-15 23:12:49]

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:87:
ERROR_PNACL_PEXE_FETCH_ABORTED = 59,
Only renumber if you're fine with this data being poisoned.  I realize you don't
have any samples, so the point is probably moot, but you should note your
reasoning in the commit message.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:105:
ERROR_PNACL_LLC_STREAM_INIT = 1,
It's unclear to me if these enums belong here (or if they don't, just raising a
question).  Would they work better in the coordinator?

Also, if they aren't used, take them out of the CL.

In any case you should stick the scary "reported to UMA" comments on these. 
Throwing in a "SUCCESS = 0" might also be useful.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:722:
"PnaclCoordinator: pexe load failed.  Aborted.");
No french spacing.  I'd suggesting making it a single phrase "pexe load was
aborted." Or something like that.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:726:
"PnaclCoordinator: pexe load failed.  No access.");
Ditto.

[jvoung (off chromium), 2013-01-16 01:31:24]

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:87:
ERROR_PNACL_PEXE_FETCH_ABORTED = 59,
On 2013/01/15 23:12:49, Nick Bray (chromium) wrote:
> Only renumber if you're fine with this data being poisoned.  I realize you
don't
> have any samples, so the point is probably moot, but you should note your
> reasoning in the commit message.

Done.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:105:
ERROR_PNACL_LLC_STREAM_INIT = 1,
On 2013/01/15 23:12:49, Nick Bray (chromium) wrote:
> It's unclear to me if these enums belong here (or if they don't, just raising
a
> question).  Would they work better in the coordinator?

Perhaps they can be moved to a pnacl specific file, and perhaps the helper
functions for pnacl in the other CL could be moved to the new file also.

> 
> Also, if they aren't used, take them out of the CL.

They're not used yet, since we still need to figure out how to propagate these
errors.  In fact, I don't think the linker returns anything right now.

I'll remove this and add them in a later CL.  Just wanted some comments on how
to do this, since leaving it with the plugin-specific error code namespace would
mean shifting things again, once we do implement these.


> 
> In any case you should stick the scary "reported to UMA" comments on these. 
> Throwing in a "SUCCESS = 0" might also be useful.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:722:
"PnaclCoordinator: pexe load failed.  Aborted.");
On 2013/01/15 23:12:49, Nick Bray (chromium) wrote:
> No french spacing.  I'd suggesting making it a single phrase "pexe load was
> aborted." Or something like that.

Pardon my french spacing.  Done. I mean... done.

https://codereview.chromium.org/11881038/diff/12001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:726:
"PnaclCoordinator: pexe load failed.  No access.");
On 2013/01/15 23:12:49, Nick Bray (chromium) wrote:
> Ditto.

Done -- cleaned up some of the other instances while I was at it.

[Nick Bray (chromium), 2013-01-16 06:01:42]

LGTM

[Derek Schuff, 2013-01-16 16:50:51]

lgtm

https://codereview.chromium.org/11881038/diff/6002/ppapi/native_client/tests/...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/11881038/diff/6002/ppapi/native_client/tests/...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:29: pnacl_refinement;
might make more sense to put the parens and the period here instead of in the
input string

[jvoung (off chromium), 2013-01-16 17:30:30]

https://codereview.chromium.org/11881038/diff/6002/ppapi/native_client/tests/...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/11881038/diff/6002/ppapi/native_client/tests/...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:29: pnacl_refinement;
On 2013/01/16 16:50:51, Derek Schuff wrote:
> might make more sense to put the parens and the period here instead of in the
> input string

Done.

[commit-bot: I haz the power, 2013-01-16 18:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/11881038/23002

[commit-bot: I haz the power, 2013-01-16 21:13:14]

Change committed as 177220