Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15961006:
Regenerate Extensions API bindings when optional permissions change (Closed)
|
Created:
7 years, 6 months ago by cduvall Modified:
7 years, 6 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRegenerate Extensions API bindings when optional permissions change
Each time the optional permissions change for an extension, the API bindings
will be regenerated, reflecting the new permissions.
R=kalman@chromium.org
BUG=55316
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204492
Patch Set 1 : #
Total comments: 19
Patch Set 2 : fixes #
Total comments: 24
Patch Set 3 : comments #Patch Set 4 : dumb mistake #Patch Set 5 : add IsEmpty check #Patch Set 6 : add missing stuff to android stubs #Patch Set 7 : rebase #Patch Set 8 : try againto fix android #
Messages
Total messages: 18 (0 generated)
This passes all the *OptionalPermission* tests, but I'm getting the context the wrong way. See comment in dispatcher.cc. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1290: // TODO(cduvall): This is wrong. Not sure what to do. I'm not sure how to get the right context to register the bindings in. I tried v8_context_set().GetCurrent(), but that was returning NULL since this isn't called from a JS context.
awesome! pretty trivial comments. ping me if you're blocked on the ForEach thing and I'll submit it (or you can pull it out yourself). https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:722: ModuleSystem* module_system, actually module_system on here isn't necessary because it's owned by ChomeV8Context. make it easier to Bind. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:743: void Dispatcher::RemoveAPIBindingIfPresent(const std::string& api_name, Maybe DeregisterBinding as a parallel to RegisterBinding. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:749: bind_object->Delete(v8::String::New(bind_name.c_str())); save a reference to the v8::String::New here. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:750: } no {} https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:754: std::string* bind_name, this is really GetOrCreateBindObject, though maybe it should be GetOrCreateBindObjectIfAvailable. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:760: v8::Handle<v8::Object> bind_object = GetOrCreateChrome(context->v8_context()); I think you'll need to move this to after the availability of things are checked, or GetBindObject will end up creating a chrome object unnecessarily. just declare then before the bind_object = GetOrCreateObject perhaps check bind_object.IsEmpty and assign to GetOrCreateChrome. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1290: // TODO(cduvall): This is wrong. Not sure what to do. On 2013/05/31 03:06:34, cduvall wrote: > I'm not sure how to get the right context to register the bindings in. I tried > v8_context_set().GetCurrent(), but that was returning NULL since this isn't > called from a JS context. Ah right, yeah. So what you want is this: https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension... which hopefully will be submitted by today, so will be ready to use soon. you'd call that on each context. If not I can just submit it separately. in fact after this it becomes super simple, probably only 1 line, so might as well just move it into where it's called (v8_context_set_.ForEach(extension->id(), base::Bind(&RegisterSchemaGeneratedBindings)). and in fact, you might want to rename RegisterSchemaGeneratedBindings to AddOrRemoveBindings or something, a nice parallel to AddOrRemovePermissions. and one more thing - it would be an interesting test here for optional permissions to also make sure that they're updated for iframes (or more generally, other contexts within the same process, and iframes are an easy way to get that - it's also more of an interesting case than an options page). it would be good to test this with something that supports optional permissions and is available to an iframe context, though, as well as bookmarks which is available to neither. I think the only thing that fits this is "storage", even though it shouldn't. anyway, whatever. maybe just make sure it works by hand and we can worry about a regression test later, let's get this stuff in. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1294: context->v8_context()->Exit(); use v8::Context::Scope here, and move into RegisterSchemaGeneratedBindings (may also need a HandleScope there).
This patch is diffed against https://codereview.chromium.org/15855010/. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:722: ModuleSystem* module_system, On 2013/05/31 16:35:00, kalman wrote: > actually module_system on here isn't necessary because it's owned by > ChomeV8Context. make it easier to Bind. Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:743: void Dispatcher::RemoveAPIBindingIfPresent(const std::string& api_name, On 2013/05/31 16:35:00, kalman wrote: > Maybe DeregisterBinding as a parallel to RegisterBinding. Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:749: bind_object->Delete(v8::String::New(bind_name.c_str())); On 2013/05/31 16:35:00, kalman wrote: > save a reference to the v8::String::New here. Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:750: } On 2013/05/31 16:35:00, kalman wrote: > no {} Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:754: std::string* bind_name, On 2013/05/31 16:35:00, kalman wrote: > this is really GetOrCreateBindObject, though maybe it should be > GetOrCreateBindObjectIfAvailable. Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:760: v8::Handle<v8::Object> bind_object = GetOrCreateChrome(context->v8_context()); On 2013/05/31 16:35:00, kalman wrote: > I think you'll need to move this to after the availability of things are > checked, or GetBindObject will end up creating a chrome object unnecessarily. > > just declare then before the bind_object = GetOrCreateObject perhaps check > bind_object.IsEmpty and assign to GetOrCreateChrome. Done. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1290: // TODO(cduvall): This is wrong. Not sure what to do. On 2013/05/31 16:35:00, kalman wrote: > On 2013/05/31 03:06:34, cduvall wrote: > > I'm not sure how to get the right context to register the bindings in. I tried > > v8_context_set().GetCurrent(), but that was returning NULL since this isn't > > called from a JS context. > > Ah right, yeah. So what you want is this: > > https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension... > > which hopefully will be submitted by today, so will be ready to use soon. you'd > call that on each context. If not I can just submit it separately. > > in fact after this it becomes super simple, probably only 1 line, so might as > well just move it into where it's called > (v8_context_set_.ForEach(extension->id(), > base::Bind(&RegisterSchemaGeneratedBindings)). > > and in fact, you might want to rename RegisterSchemaGeneratedBindings to > AddOrRemoveBindings or something, a nice parallel to AddOrRemovePermissions. > > and one more thing - it would be an interesting test here for optional > permissions to also make sure that they're updated for iframes (or more > generally, other contexts within the same process, and iframes are an easy way > to get that - it's also more of an interesting case than an options page). it > would be good to test this with something that supports optional permissions and > is available to an iframe context, though, as well as bookmarks which is > available to neither. I think the only thing that fits this is "storage", even > though it shouldn't. > > anyway, whatever. maybe just make sure it works by hand and we can worry about a > regression test later, let's get this stuff in. So I added in the v8_context_set_.ForEach(), but I'm getting a weird crash in v8 code: # # Fatal error in ../../v8/src/isolate.cc, line 991 # CHECK(context()) failed # I didn't have time to fix it today, but I'll look into it more tomorrow. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1294: context->v8_context()->Exit(); On 2013/05/31 16:35:00, kalman wrote: > use v8::Context::Scope here, and move into RegisterSchemaGeneratedBindings (may > also need a HandleScope there). Done.
lgtm if it's just trivial changes, ping it back to me if not. https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1290: // TODO(cduvall): This is wrong. Not sure what to do. It may be the lack of an is_valid check in ChromeV8ContextSet::ForEach. Could you add it? I.e. 88 for (ContextSet::iterator it = contexts.begin(); it != contexts.end(); 89 ++it) { if (!(*it)->is_valid()) continue; 90 if (!extension_id.empty()) { ... you'll need to make is_valid part of the public API for ContextSet. if that doesn't fix the problem - well, we still need it. If not - the crash is in isolate.cc and has the following comment: 989 // Check for compatibility between the security tokens in the 990 // current lexical context and the accessed object. 991 ASSERT(context()); so it may be something to do with security tokens i.e. cross-context access? maybe the context isn't getting set up properly? eh I don't really know, I can fiddle with it too if you tell me how to repro the crash. https://codereview.chromium.org/15961006/diff/26001/chrome/common/extensions/... File chrome/common/extensions/features/permission_feature.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/common/extensions/... chrome/common/extensions/features/permission_feature.cc:31: !PermissionsData::GetActivePermissions(extension)-> you can just remove this, HasAPIPermission checks the active permissions. then it's a 1-line if statement without the {}. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:703: v8::HandleScope handle_scope; we shuold remove the handle scope here, misleading and unnecessary https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:723: v8::Context::Scope(context->v8_context()); nit: new line after the scope declarations make it easier to see that they're there. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:745: v8::HandleScope handle_scope; handle scope shouldn't be necessary here, callers have it https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:758: v8::HandleScope handle_scope; handle scope shouldn't be necessary here, callers have it https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:806: CHECK(module_system); myeh, no point CHECKing here. maybe a DCHECK if you think it's worthwhile. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1060: context->set_module_system(module_system.Pass()); to prevent callers from using the module system accidentally: { scoped_ptr<ModuleSystem> module_system(...); context->set_module_system(...); } then to reduce the diff here you could assign it again: ModuleSystem* module_system - context->module_system(); https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/permissions/optional/background.js (right): https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:127: assertTrue(chrome.bookmarks === undefined); assertEq(undefined, chrome.bookmarks); https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:196: assertTrue(chrome.bookmarks === undefined); ditto https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:290: assertTrue(chrome.bookmarks === undefined); ditto https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/permissions/optional_deny/background.js (right): https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional_deny/background.js:51: assertTrue(chrome.bookmarks === undefined); ditto
https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/2001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:1290: // TODO(cduvall): This is wrong. Not sure what to do. On 2013/06/05 19:03:44, kalman wrote: > It may be the lack of an is_valid check in ChromeV8ContextSet::ForEach. Could > you add it? I.e. > > 88 for (ContextSet::iterator it = contexts.begin(); it != contexts.end(); > 89 ++it) { > if (!(*it)->is_valid()) > continue; > > 90 if (!extension_id.empty()) { > ... > > you'll need to make is_valid part of the public API for ContextSet. > > if that doesn't fix the problem - well, we still need it. > > If not - > > the crash is in isolate.cc and has the following comment: > > 989 // Check for compatibility between the security tokens in the > 990 // current lexical context and the accessed object. > 991 ASSERT(context()); > > so it may be something to do with security tokens i.e. cross-context access? > maybe the context isn't getting set up properly? > > eh I don't really know, I can fiddle with it too if you tell me how to repro the > crash. Looks like the is_valid() check didn't fix the problem. I'm going to keep working on it but upload what I have, if you want to take a look. Repro by running ExtensionApiTest.OptionalPermissionsGranted https://codereview.chromium.org/15961006/diff/26001/chrome/common/extensions/... File chrome/common/extensions/features/permission_feature.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/common/extensions/... chrome/common/extensions/features/permission_feature.cc:31: !PermissionsData::GetActivePermissions(extension)-> On 2013/06/05 19:03:44, kalman wrote: > you can just remove this, HasAPIPermission checks the active permissions. > > then it's a 1-line if statement without the {}. Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:703: v8::HandleScope handle_scope; On 2013/06/05 19:03:44, kalman wrote: > we shuold remove the handle scope here, misleading and unnecessary Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:723: v8::Context::Scope(context->v8_context()); On 2013/06/05 19:03:44, kalman wrote: > nit: new line after the scope declarations make it easier to see that they're > there. Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:745: v8::HandleScope handle_scope; On 2013/06/05 19:03:44, kalman wrote: > handle scope shouldn't be necessary here, callers have it Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:758: v8::HandleScope handle_scope; On 2013/06/05 19:03:44, kalman wrote: > handle scope shouldn't be necessary here, callers have it Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:806: CHECK(module_system); On 2013/06/05 19:03:44, kalman wrote: > myeh, no point CHECKing here. maybe a DCHECK if you think it's worthwhile. Done. https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1060: context->set_module_system(module_system.Pass()); On 2013/06/05 19:03:44, kalman wrote: > to prevent callers from using the module system accidentally: > > { > scoped_ptr<ModuleSystem> module_system(...); > context->set_module_system(...); > } > > then to reduce the diff here you could assign it again: > > ModuleSystem* module_system - context->module_system(); Done. https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/permissions/optional/background.js (right): https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:127: assertTrue(chrome.bookmarks === undefined); On 2013/06/05 19:03:44, kalman wrote: > assertEq(undefined, chrome.bookmarks); Done. https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:196: assertTrue(chrome.bookmarks === undefined); On 2013/06/05 19:03:44, kalman wrote: > ditto Done. https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional/background.js:290: assertTrue(chrome.bookmarks === undefined); On 2013/06/05 19:03:44, kalman wrote: > ditto Done. https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/permissions/optional_deny/background.js (right): https://codereview.chromium.org/15961006/diff/26001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/permissions/optional_deny/background.js:51: assertTrue(chrome.bookmarks === undefined); On 2013/06/05 19:03:44, kalman wrote: > ditto Done.
https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:723: v8::Context::Scope(context->v8_context()); Strangely, if I take out the v8::Context::Scope, and just manually Enter() and Exit() context->v8_context() at the beginning and end of this function, I don't get the crash in isolate.cc. v8::Context::Scope does literally just that, so I have no idea why manually doing it works.
https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/15961006/diff/26001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:723: v8::Context::Scope(context->v8_context()); On 2013/06/06 00:01:41, cduvall wrote: > Strangely, if I take out the v8::Context::Scope, and just manually Enter() and > Exit() context->v8_context() at the beginning and end of this function, I don't > get the crash in isolate.cc. v8::Context::Scope does literally just that, so I > have no idea why manually doing it works. Wow, looks like I'm a dummy. This line should be: v8::Context::Scope scope(context->v8_context()); not: v8::Context::Scope(context->v8_context()); Sorry about that one.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cduvall@chromium.org/15961006/44001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cduvall@chromium.org/15961006/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... 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.
Ok looks like we need to add this methods into chrome/common/extensions/api/extension_api_stub.cc
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cduvall@chromium.org/15961006/67001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cduvall@chromium.org/15961006/79001
(lgtm I added the missing stubs for you - I want to land this!)
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... 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/cduvall@chromium.org/15961006/97001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cduvall@chromium.org/15961006/97001
Message was sent while issue was closed.
Change committed as 204492 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
