Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338353008:
NaCl: clean up nexe loading logic in trusted plugin. (Closed)
|
Created:
6 years, 6 months ago by Nick Bray (chromium) Modified:
6 years, 6 months ago CC:
chromium-reviews, native-client-reviews_googlegroups.com, hidehiko Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionNaCl: clean up nexe loading logic in trusted plugin.
CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279762
Patch Set 1 #
Total comments: 13
Patch Set 2 : Edits #Patch Set 3 : More edits #Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : EDits #Patch Set 6 : Rebase + resolve conflicts #
Messages
Total messages: 23 (0 generated)
A quick FYI before I come back and give this a more thorough review. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:710: if (!StartModule()) { We had initially planned on LoadModule() being asynchronous and sending messages over Chrome IPC, though that changed to no longer be necessary. However, I'm not sure yet whether or not StartModule will happen over Chrome IPC (or whether it'll just happen as part of StartSelLdr). I suppose this change is fine for now, though StartModule and the methods that call it might have to be made asynchronous again.
https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:710: if (!StartModule()) { On 2014/06/18 01:37:52, teravest wrote: > We had initially planned on LoadModule() being asynchronous and sending messages > over Chrome IPC, though that changed to no longer be necessary. > > However, I'm not sure yet whether or not StartModule will happen over Chrome IPC > (or whether it'll just happen as part of StartSelLdr). I suppose this change is > fine for now, though StartModule and the methods that call it might have to be > made asynchronous again. I think the rest of the cleanup is still good (?), even if I put the callback back in (the callback would only happen in one place, now... previously the callback can either be invoked synchonously or asynchronously. Joy.) Tell me what you want. The only real trick would be sorting out the weakref/lifetime issues.
On Tue, Jun 17, 2014 at 8:02 PM, <ncbray@chromium.org> wrote: > > https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... > File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): > > https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... > ppapi/native_client/src/trusted/plugin/service_runtime.cc:710: if > (!StartModule()) { > On 2014/06/18 01:37:52, teravest wrote: >> >> We had initially planned on LoadModule() being asynchronous and > > sending messages >> >> over Chrome IPC, though that changed to no longer be necessary. > > >> However, I'm not sure yet whether or not StartModule will happen over > > Chrome IPC >> >> (or whether it'll just happen as part of StartSelLdr). I suppose this > > change is >> >> fine for now, though StartModule and the methods that call it might > > have to be >> >> made asynchronous again. > > > I think the rest of the cleanup is still good (?), even if I put the > callback back in (the callback would only happen in one place, now... > previously the callback can either be invoked synchonously or > asynchronously. Joy.) Tell me what you want. The only real trick > would be sorting out the weakref/lifetime issues. Yeah, the rest of the cleanup is still good. I suspect (right now) that StartModule will have to be async, as will be the functions that call it, so it'd be nice to not make that any harder than it is now. > > https://codereview.chromium.org/338353008/ -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On Tue, Jun 17, 2014 at 8:02 PM, <ncbray@chromium.org> wrote: > > https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... > File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): > > https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... > ppapi/native_client/src/trusted/plugin/service_runtime.cc:710: if > (!StartModule()) { > On 2014/06/18 01:37:52, teravest wrote: >> >> We had initially planned on LoadModule() being asynchronous and > > sending messages >> >> over Chrome IPC, though that changed to no longer be necessary. > > >> However, I'm not sure yet whether or not StartModule will happen over > > Chrome IPC >> >> (or whether it'll just happen as part of StartSelLdr). I suppose this > > change is >> >> fine for now, though StartModule and the methods that call it might > > have to be >> >> made asynchronous again. > > > I think the rest of the cleanup is still good (?), even if I put the > callback back in (the callback would only happen in one place, now... > previously the callback can either be invoked synchonously or > asynchronously. Joy.) Tell me what you want. The only real trick > would be sorting out the weakref/lifetime issues. Yeah, the rest of the cleanup is still good. I suspect (right now) that StartModule will have to be async, as will be the functions that call it, so it'd be nice to not make that any harder than it is now. > > https://codereview.chromium.org/338353008/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+hidehiko, who is touching the load path for Non-SFI.
https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:119: info nit: )); should be in this line. There is no specific statement in the style guide, but most code in chromium and style guide follows it, so it'd be better to follow it. cf) http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (left): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:497: NaClLog(4, "ServiceRuntime::LoadNexeAndStart (success)\n"); Probably we want to keep this lo, too. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:657: nexe_started_ = true; Probably we should rename this variable (and the method name, too?). If LoadNexeAndStartInternal fails, actually nexe is not started, but nexe_started_ is expected to be set to true even such a case. Sounds confusing. Also, nexe_started_ and nexe_started_ok_ sounds confusing, because those "name"s look same meaning. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:692: ErrorInfo error_info; Can we avoid dup? Probably, we should refactor LoadModule() out, similar to InitReverseService/StartModule etc. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:724: NaClLog(LOG_ERROR, "reap logs\n"); Why not fatal? https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.h:188: bool WaitForNexeStart(); Could you document the return value?
Mechanical changes done, but one of the comments made me realize I need to dig deeper into RPC weirdness. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:119: info On 2014/06/18 05:07:04, hidehiko wrote: > nit: )); should be in this line. > There is no specific statement in the style guide, > but most code in chromium and style guide follows it, so it'd be better to > follow it. > > cf) http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml Done. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:657: nexe_started_ = true; On 2014/06/18 05:07:04, hidehiko wrote: > Probably we should rename this variable (and the method name, too?). > If LoadNexeAndStartInternal fails, actually nexe is not started, but > nexe_started_ is expected to be set to true even such a case. Sounds confusing. > Also, nexe_started_ and nexe_started_ok_ sounds confusing, because those "name"s > look same meaning. Done, but please provide suggestions when names are bad. I find it hard to come up with good ones on my own, and a second brain is always welcome. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:692: ErrorInfo error_info; On 2014/06/18 05:07:04, hidehiko wrote: > Can we avoid dup? > > Probably, we should refactor LoadModule() out, similar to > InitReverseService/StartModule etc. Done. Good call. I am not 100% happy with reporting the error in this method once the functionality is factored out because it breaks the pattern of the callee doing error reporting, but... I have no better idea. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.cc:724: NaClLog(LOG_ERROR, "reap logs\n"); On 2014/06/18 05:07:04, hidehiko wrote: > Why not fatal? Oh. This is even more subtle than I realized. It originally called "Log" which I though was digging around with the internals of NaClLog... but it looks like it was calling a method on the class which makes an SRPC request which indirectly kills the sel_ldr process. (Which I believe causes flakiness for some of the "bad" tests because the plugin does not immediately detect the sel_ldr process has died... :/) I need to look at this again. https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/338353008/diff/1/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/service_runtime.h:188: bool WaitForNexeStart(); On 2014/06/18 05:07:04, hidehiko wrote: > Could you document the return value? Done.
For the record, it looks like LOG_FATAL in the trusted NaCl process is triggering the OS crash catcher on OSX. Which it shouldn't, but hey. Recursing and trying to figure out why.
PTAL Moved the test-reenabling to this CL, which also fixes the flake caused by LOG_FATAL. https://codereview.chromium.org/347803004/
LGTM with nit picks. Please wait for Mark and Justin's review. Could you run linux_rel_precise32 trybot, too? Also, would you mind to add CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32 to your CL description, so that the bot will be run on CQ try. Thanks, - hidehiko https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:194: void LoadNexeAndStart(PP_NaClFileInfo file_info); nit: Could you also add short comment that this function can be called only once? Otherwise, your change around SignalNexeStarted() won't work. https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:216: bool LoadNexeAndStartInternal(PP_NaClFileInfo file_info); nit: "const PP_NaClFileInfo&". https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:220: bool LoadModule(PP_NaClFileInfo file_info); nit: "const PP_NaClFileInfo&".
lgtm
https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:194: void LoadNexeAndStart(PP_NaClFileInfo file_info); On 2014/06/20 04:21:38, hidehiko wrote: > nit: Could you also add short comment that this function can be called only > once? Otherwise, your change around SignalNexeStarted() won't work. Done. https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:216: bool LoadNexeAndStartInternal(PP_NaClFileInfo file_info); On 2014/06/20 04:21:38, hidehiko wrote: > nit: "const PP_NaClFileInfo&". Done. https://codereview.chromium.org/338353008/diff/60001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.h:220: bool LoadModule(PP_NaClFileInfo file_info); On 2014/06/20 04:21:38, hidehiko wrote: > nit: "const PP_NaClFileInfo&". Done.
Rebased and resolved conflicts with hidehiko's CL. If anyone wants to make sure I didn't miss anything when merging it would be appreciated, otherwise I'll commit later today.
The CQ bit was checked by ncbray@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncbray@chromium.org/338353008/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by ncbray@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncbray@chromium.org/338353008/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 279762 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
