Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14860020:
chrome://nacl asserts (Closed)
|
Created:
7 years, 7 months ago by yael.aharon Modified:
7 years, 7 months ago Reviewers:
Dan Beam, yael.aharon1, Evan Stade, jvoung (off chromium), arv (Not doing code reviews), jvoung - send to chromium..., James Hawkins CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionchrome://nacl asserts
A check was added in r180577 to check that the pnacl directory exists. The check runs in the UI thread and calls file_util::PathExists().
The UI thread does not have permission to make this call, and it asserts in ThreadRestrictions::AssertIOAllowed
Create a NaClDOMHandlerProxy class that performs this check in the FILE thread.
BUG=240013
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200669
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix style issues #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 19
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : Fix the clang build #Patch Set 9 : #Patch Set 10 : #
Total comments: 10
Messages
Total messages: 31 (0 generated)
Can you please review? thanks!
Thanks for fixing this! Some style fixes. https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:78: void validatePnaclPath(); style: Method names start with capital letter? https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:83: // A helper callback that receives the result of checking if pnacl path In the comments, could you make it "PNaCl" instead of "pnacl"? https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:109: void validatePnaclPathCallback(bool is_valid); style: Method names start with capital letter https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:144: { put curly brace on previous line https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:165: { same (curly) https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:193: if (proxy_) { indentation 2 spaces instead of 4 https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:339: { style: put curly on the previous line https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... chrome/browser/ui/webui/nacl_ui.cc:352: DCHECK(proxy_); make DCHECK indentation consistent with the rest of the block
On 2013/05/15 20:28:28, jvoung (cr) wrote: > Thanks for fixing this! Some style fixes. > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > File chrome/browser/ui/webui/nacl_ui.cc (right): > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:78: void validatePnaclPath(); > style: Method names start with capital letter? > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:83: // A helper callback that receives the > result of checking if pnacl path > In the comments, could you make it "PNaCl" instead of "pnacl"? > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:109: void validatePnaclPathCallback(bool > is_valid); > style: Method names start with capital letter > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:144: { > put curly brace on previous line > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:165: { > same (curly) > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:193: if (proxy_) { > indentation 2 spaces instead of 4 > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:339: { > style: put curly on the previous line > > https://codereview.chromium.org/14860020/diff/1/chrome/browser/ui/webui/nacl_... > chrome/browser/ui/webui/nacl_ui.cc:352: DCHECK(proxy_); > make DCHECK indentation consistent with the rest of the block Thanks for reviewing, I am sorry for the style violations, will fix them right away.
Fixed the style issues, could you please have another look? thanks
https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... chrome/browser/ui/webui/nacl_ui.cc:80: void setHandler(NaClDOMHandler* handler); For chromium, I think the style for setters and getters is like: void set_foo(...) { foo_ = ...; } so you could make set_handler(...) { ... } here https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... chrome/browser/ui/webui/nacl_ui.cc:160: handler_->ValidatePnaclPathCallback(is_valid); Shouldn't this call the proxy's ValidatePnaclPathCallback to switch back to the UI thread, instead of the handler's? Actually, is it possible to name those two callbacks differently to reduce this confusion? Maybe one of them could be called "DidValidatePnaclPath(...)" instead? https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... chrome/browser/ui/webui/nacl_ui.cc:175: void NaClDOMHandlerProxy::setHandler(NaClDOMHandler* handler) { move set_handler inline
On 2013/05/15 21:19:30, jvoung (cr) wrote: > https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... > File chrome/browser/ui/webui/nacl_ui.cc (right): > > https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... > chrome/browser/ui/webui/nacl_ui.cc:80: void setHandler(NaClDOMHandler* handler); > For chromium, I think the style for setters and getters is like: > > void set_foo(...) { foo_ = ...; } > > so you could make set_handler(...) { ... } here > > https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... > chrome/browser/ui/webui/nacl_ui.cc:160: > handler_->ValidatePnaclPathCallback(is_valid); > Shouldn't this call the proxy's ValidatePnaclPathCallback to switch back to the > UI thread, instead of the handler's? > > Actually, is it possible to name those two callbacks differently to reduce this > confusion? Maybe one of them could be called "DidValidatePnaclPath(...)" > instead? > Sorry, using the same name was a bad idea :( > https://codereview.chromium.org/14860020/diff/5001/chrome/browser/ui/webui/na... > chrome/browser/ui/webui/nacl_ui.cc:175: void > NaClDOMHandlerProxy::setHandler(NaClDOMHandler* handler) { > move set_handler inline
Updated based on last comments, thanks
lgtm https://codereview.chromium.org/14860020/diff/6002/chrome/browser/ui/webui/na... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/6002/chrome/browser/ui/webui/na... chrome/browser/ui/webui/nacl_ui.cc:80: void set_handler(NaClDOMHandler* handler){ space between ) and { https://codereview.chromium.org/14860020/diff/6002/chrome/browser/ui/webui/na... chrome/browser/ui/webui/nacl_ui.cc:110: // exists. It switches back to the UI thread and saves the result. Update comment: this one doesn't try to switch back to the UI thread.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14860020/15001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
I am sorry, I thought one lgtm was enough. Who is the OWNER for this file?
Can I please get an OWNER to review this? thanks
https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:72: class NaClDOMHandlerProxy : public nit: Too many spaces after colon. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:72: class NaClDOMHandlerProxy : public nit: Document class. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:73: base::RefCountedThreadSafe<NaClDOMHandlerProxy> { nit: Indentation is off. Should be 4 spaces. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:76: // A helper to check if PNaCl path exists. nit: Add blank line above this. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:77: // The check is done on the FILE thread. nit: Remove this part of the documentation; it's an implementation detail. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:86: // exists. It switches back to the UI thread and saves the result. nit: Remove the last part of the documentation; it's an implementation detail. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:89: NaClDOMHandler* handler_; nit: Document member variable. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:111: void DidValidatePnaclPath(bool is_valid); nit: Document parameter. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:190: proxy_->set_handler(NULL); Is there a race here? NaClDOMHandler is destroyed before ValidatePnaclPathCallback is called on the other thread. handler_ will be NULL in said method.
On 2013/05/15 22:15:19, James Hawkins wrote: > Thanks for reviewing! The chromium style is new to me so please forgive my mistakes here. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... > chrome/browser/ui/webui/nacl_ui.cc:190: proxy_->set_handler(NULL); > Is there a race here? NaClDOMHandler is destroyed before > ValidatePnaclPathCallback is called on the other thread. handler_ will be NULL > in said method. Yes, I should add a check before using handler_. Thanks for spotting that.
I updated the patch based on your latest comments, please review again :) thanks.
On 2013/05/16 01:10:16, yael.aharon1 wrote: > I updated the patch based on your latest comments, please review again :) > thanks. Please go into each comment you resolved and press the 'Done' button, then publish messages (so I can see exactly what you've changed).
I addressed all your comments, and now I marked them as done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:72: class NaClDOMHandlerProxy : public On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Too many spaces after colon. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:72: class NaClDOMHandlerProxy : public On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Document class. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:73: base::RefCountedThreadSafe<NaClDOMHandlerProxy> { On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Indentation is off. Should be 4 spaces. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:76: // A helper to check if PNaCl path exists. On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Add blank line above this. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:77: // The check is done on the FILE thread. On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Remove this part of the documentation; it's an implementation detail. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:86: // exists. It switches back to the UI thread and saves the result. On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Remove the last part of the documentation; it's an implementation detail. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:89: NaClDOMHandler* handler_; On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Document member variable. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:111: void DidValidatePnaclPath(bool is_valid); On 2013/05/15 22:15:20, James Hawkins wrote: > nit: Document parameter. Done. https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:190: proxy_->set_handler(NULL); On 2013/05/15 22:15:20, James Hawkins wrote: > Is there a race here? NaClDOMHandler is destroyed before > ValidatePnaclPathCallback is called on the other thread. handler_ will be NULL > in said method. Done.
https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/15001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:72: class NaClDOMHandlerProxy : public On 2013/05/16 17:39:26, yael.aharon wrote: > On 2013/05/15 22:15:20, James Hawkins wrote: > > nit: Document class. > > Done. I don't see the class documented in the latest draft. https://codereview.chromium.org/14860020/diff/22001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/22001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:176: if (handler_) nit: Document why this check is required.
Done, and added the class documentation which I thought I added yesterday, but didn't. https://codereview.chromium.org/14860020/diff/22001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/22001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:176: if (handler_) On 2013/05/16 17:42:18, James Hawkins wrote: > nit: Document why this check is required. Done.
LGTM with nit. https://codereview.chromium.org/14860020/diff/28001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/28001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:197: if (proxy_) { nit: Be consistent: You don't use braces for single line if block above.
https://codereview.chromium.org/14860020/diff/28001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/28001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:197: if (proxy_) { On 2013/05/16 17:56:37, James Hawkins wrote: > nit: Be consistent: You don't use braces for single line if block above. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14860020/33001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14860020/35003
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14860020/51002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14860020/36006
lgtm w/nits https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/nacl_ui.cc (right): https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:70: class NaClDOMHandler; opt nit: NaclDomHandler https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:76: base::RefCountedThreadSafe<NaClDOMHandlerProxy> { class NaClDOMHandlerProxy : public base::RefCountedThreadSafe<NaClDOMHandlerProxy> { https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:83: void set_handler(NaClDOMHandler* handler) { void set_handler(NaClDOMHandler* handler) { handler_ = handler; } https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:92: virtual ~NaClDOMHandlerProxy() {} put dtor second https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:93: friend class base::RefCountedThreadSafe<NaClDOMHandlerProxy>; put friend first https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:96: NaClDOMHandler* handler_; NaclDomHandler* handler_; // weak. (assuming this is weak) https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:117: // exists. is_valid is true if the PNaCl path that was returned by nit: |is_valid| https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:167: if (!got_path || pnacl_path.empty() || !file_util::PathExists(pnacl_path)) { nit: no curlies https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:170: ValidatePnaclPathCallback(is_valid); nit: ValidatePnaclPathCallback( got_path && !pnacl_path.empty() && file_util::PathExists(pnacl_path)); (IMO) https://codereview.chromium.org/14860020/diff/36006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/nacl_ui.cc:194: proxy_(new NaClDOMHandlerProxy(this)) { ALLOW_THIS_IN_INITIALIZER_LIST()
Message was sent while issue was closed.
Change committed as 200669
Message was sent while issue was closed.
Dan, thank you for your review. I will create a new CL to address it. I cannot find any code that defines or uses ALLOW_THIS_IN_INITIALIZER_LIST(). In the ppapi code, I see PP_ALLOW_THIS_IN_INITIALIZER_LIST, but this change is not in ppapi. Please advice if you think this change is needed. thanks.
Message was sent while issue was closed.
On 2013/05/17 01:43:33, yael.aharon1 wrote: > Dan, thank you for your review. I will create a new CL to address it. > I cannot find any code that defines or uses ALLOW_THIS_IN_INITIALIZER_LIST(). > In the ppapi code, I see PP_ALLOW_THIS_IN_INITIALIZER_LIST, but this change is > not in ppapi. > Please advice if you think this change is needed. > thanks. my bad, this was removed a while ago - http://src.chromium.org/viewvc/chrome?revision=197671&view=revision - sorry about that |
