Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10898034:
Disallow NaCl in chrome webstore for ARM (Closed)
|
Created:
8 years, 3 months ago by Derek Schuff Modified:
8 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org, bbudge-google Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisallow NaCl in chrome webstore for ARM
The ARM NaCl ABI will most likely change slightly soon
so don't allow arm nacl modules from the webstore for M23.
R=sehr@chromium.org,brettw@chromium.org
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154053
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 15 (0 generated)
Pretty trivial patch if we want to allow LOAD as well as COMPONENT. (Brett, I belive I need someone from chrome/OWNERS since chrome/renderer does not have an OWNERS file. Congratulations!)
+cc bbudge, original author of this code
Does "chrome/browser/extensions/extension_nacl_browsertest.cc" run on ARM? https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:608: #ifdef __arm__ nit: not sure if the if-defs are usually ARCH_CPU_ARM_FAMILY or __arm__ in chrome code (lot of third_party uses __arm__, while base/ uses the other...) https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:611: bool is_nacl_allowed = is_extension_unrestricted || is_nacl_unrestricted; Just to confirm, some of the apps we are interested in testing are Component/Load ones, and don't require other whitelisting?
https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:608: #ifdef __arm__ Yeah, I wasn't sure either. I found both sprinkled in various places throughout the chrome code (and both in content/ and chrome/), but more uses of __arm__ so I went with that. On 2012/08/29 16:49:33, jvoung (chromium) wrote: > nit: not sure if the if-defs are usually ARCH_CPU_ARM_FAMILY or __arm__ in > chrome code (lot of third_party uses __arm__, while base/ uses the other...) https://chromiumcodereview.appspot.com/10898034/diff/2001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:611: bool is_nacl_allowed = is_extension_unrestricted || is_nacl_unrestricted; Right. If you look directly above the only whitelisted URLs are plus.google.com/games. So unless I hear specifically about them, they will be blocked. On 2012/08/29 16:49:33, jvoung (chromium) wrote: > Just to confirm, some of the apps we are interested in testing are > Component/Load ones, and don't require other whitelisting?
+Nico, from chrome/OWNERS (brettw seems swamped)
https://chromiumcodereview.appspot.com/10898034/diff/3006/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/10898034/diff/3006/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:619: #endif Can you write this as: bool is_nacl_allowed = #if !defined(__arm__) // The ARM ABI is not quite stable, so only allow NaCl for unr… is_extension_from_webstore || is_whitelisted_url || #endif is_extension_unrestricted || is_nacl_unrestricted;
https://chromiumcodereview.appspot.com/10898034/diff/3006/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/10898034/diff/3006/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:619: #endif On 2012/08/29 17:08:15, Nico wrote: > Can you write this as: > > bool is_nacl_allowed = > #if !defined(__arm__) > // The ARM ABI is not quite stable, so only allow NaCl for unr… > is_extension_from_webstore || > is_whitelisted_url || > #endif > is_extension_unrestricted || > is_nacl_unrestricted; Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dschuff@chromium.org/10898034/5003
Try job failure for 10898034-5003 (retry) on win_rel for step "runhooks". It's a second try, previously, steps "browser_tests, unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dschuff@chromium.org/10898034/5003
Try job failure for 10898034-5003 (retry) on linux_rel for steps "nacl_integration, interactive_ui_tests, browser_tests, sync_integration_tests". It's a second try, previously, steps "nacl_integration, interactive_ui_tests, browser_tests, sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dschuff@chromium.org/10898034/5003
Change committed as 154053 |
