Regenerate Extensions API bindings when optional permissions change

chrome/renderer/extensions/dispatcher.cc

Index: chrome/renderer/extensions/dispatcher.cc
diff --git a/chrome/renderer/extensions/dispatcher.cc b/chrome/renderer/extensions/dispatcher.cc
index a1fd9c5d3ef5f6f757ece3a37bf72c679fb7da49..bf6560f8da027476e862aa9a36cb729fbbcd32b2 100644
--- a/chrome/renderer/extensions/dispatcher.cc
+++ b/chrome/renderer/extensions/dispatcher.cc
@@ -726,64 +726,95 @@ void Dispatcher::RegisterSchemaGeneratedBindings(
   for (std::set<std::string>::iterator it = apis.begin();
        it != apis.end(); ++it) {
     const std::string& api_name = *it;
-    if (!context->IsAnyFeatureAvailableToContext(api_name))
+    if (!context->IsAnyFeatureAvailableToContext(api_name)) {
+      RemoveAPIBindingIfPresent(api_name, context);
       continue;
+    }
 
     Feature* feature =
         BaseFeatureProvider::GetByName("api")->GetFeature(api_name);
     if (feature && feature->IsInternal())
       continue;
 
-    std::vector<std::string> split;
-    base::SplitString(api_name, '.', &split);
-
-    v8::Handle<v8::Object> bind_object =
-        GetOrCreateChrome(context->v8_context());
-
-    // Check if this API has an ancestor. If the API's ancestor is available and
-    // the API is not available, don't install the bindings for this API. If
-    // the API is available and its ancestor is not, delete the ancestor and
-    // install the bindings for the API. This is to prevent loading the ancestor
-    // API schema if it will not be needed.
-    //
-    // For example:
-    //  If app is available and app.window is not, just install app.
-    //  If app.window is available and app is not, delete app and install
-    //  app.window on a new object so app does not have to be loaded.
-    std::string ancestor_name;
-    bool only_ancestor_available = false;
-    for (size_t i = 0; i < split.size() - 1; ++i) {
-      ancestor_name += (i ? ".": "") + split[i];
-      if (!ancestor_name.empty() &&
-          context->GetAvailability(ancestor_name).is_available() &&
-          !context->GetAvailability(api_name).is_available()) {
-        only_ancestor_available = true;
-        break;
-      }
-      bind_object = GetOrCreateObject(bind_object, split[i]);
-    }
-    if (only_ancestor_available)
-      continue;
+    RegisterBinding(api_name, module_system, context);
+  }
+}
 
-    if (lazy_bindings_map_.find(api_name) != lazy_bindings_map_.end()) {
-      InstallBindings(module_system, context->v8_context(), api_name);
-    } else if (!source_map_.Contains(api_name)) {
-      module_system->RegisterNativeHandler(
-          api_name,
-          scoped_ptr<NativeHandler>(new BindingGeneratingNativeHandler(
-              module_system,
-              api_name,
-              "binding")));
-      module_system->SetNativeLazyField(bind_object,
-                                        split.back(),
-                                        api_name,
-                                        "binding");
-    } else {
-      module_system->SetLazyField(bind_object,
-                                  split.back(),
-                                  api_name,
-                                  "binding");
+void Dispatcher::RemoveAPIBindingIfPresent(const std::string& api_name,

[not at google - send to devlin, 2013/05/31 16:35:00]

Maybe DeregisterBinding as a parallel to RegisterBinding.

[cduvall, 2013/06/05 03:05:56]

Done.

+                                           ChromeV8Context* context) {
+  std::string bind_name;
+  v8::Handle<v8::Object> bind_object =
+      GetBindObject(api_name, &bind_name, context);
+  if (bind_object->HasRealNamedProperty(v8::String::New(bind_name.c_str()))) {
+    bind_object->Delete(v8::String::New(bind_name.c_str()));

[not at google - send to devlin, 2013/05/31 16:35:00]

save a reference to the v8::String::New here.

[cduvall, 2013/06/05 03:05:56]

Done.

+  }

[not at google - send to devlin, 2013/05/31 16:35:00]

no {}

[cduvall, 2013/06/05 03:05:56]

Done.

+}
+
+v8::Handle<v8::Object> Dispatcher::GetBindObject(const std::string& api_name,
+                                                 std::string* bind_name,

[not at google - send to devlin, 2013/05/31 16:35:00]

this is really GetOrCreateBindObject, though maybe it should be
GetOrCreateBindObjectIfAvailable.

[cduvall, 2013/06/05 03:05:56]

Done.

+                                                 ChromeV8Context* context) {
+  v8::HandleScope handle_scope;
+  std::vector<std::string> split;
+  base::SplitString(api_name, '.', &split);
+
+  v8::Handle<v8::Object> bind_object = GetOrCreateChrome(context->v8_context());

[not at google - send to devlin, 2013/05/31 16:35:00]

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.

[cduvall, 2013/06/05 03:05:56]

Done.

+
+  // Check if this API has an ancestor. If the API's ancestor is available and
+  // the API is not available, don't install the bindings for this API. If
+  // the API is available and its ancestor is not, delete the ancestor and
+  // install the bindings for the API. This is to prevent loading the ancestor
+  // API schema if it will not be needed.
+  //
+  // For example:
+  //  If app is available and app.window is not, just install app.
+  //  If app.window is available and app is not, delete app and install
+  //  app.window on a new object so app does not have to be loaded.
+  std::string ancestor_name;
+  bool only_ancestor_available = false;
+  for (size_t i = 0; i < split.size() - 1; ++i) {
+    ancestor_name += (i ? ".": "") + split[i];
+    if (!ancestor_name.empty() &&
+        context->GetAvailability(ancestor_name).is_available() &&
+        !context->GetAvailability(api_name).is_available()) {
+      only_ancestor_available = true;
+      break;
     }
+    bind_object = GetOrCreateObject(bind_object, split[i]);
+  }
+  if (only_ancestor_available)
+    return handle_scope.Close(v8::Handle<v8::Object>());
+  if (bind_name)
+    *bind_name = split.back();
+  return handle_scope.Close(bind_object);
+}
+
+void Dispatcher::RegisterBinding(const std::string& api_name,
+                                 ModuleSystem* module_system,
+                                 ChromeV8Context* context) {
+  std::string bind_name;
+  v8::Handle<v8::Object> bind_object =
+      GetBindObject(api_name, &bind_name, context);
+  if (bind_object.IsEmpty())
+    return;
+
+  if (lazy_bindings_map_.find(api_name) != lazy_bindings_map_.end()) {
+    InstallBindings(module_system, context->v8_context(), api_name);
+  } else if (!source_map_.Contains(api_name)) {
+    module_system->RegisterNativeHandler(
+        api_name,
+        scoped_ptr<NativeHandler>(new BindingGeneratingNativeHandler(
+            module_system,
+            api_name,
+            "binding")));
+    module_system->SetNativeLazyField(bind_object,
+                                      bind_name,
+                                      api_name,
+                                      "binding");
+  } else {
+    module_system->SetLazyField(bind_object,
+                                bind_name,
+                                api_name,
+                                "binding");
   }
 }
 
@@ -1253,6 +1284,16 @@ void Dispatcher::AddOrRemoveOriginPermissions(
   }
 }
 
+void Dispatcher::RefreshSchemaGeneratedBindings() {
+  v8::HandleScope handle_scope;
+  CHECK(v8_context_set().size());
+  // TODO(cduvall): This is wrong. Not sure what to do.

[cduvall, 2013/05/31 03:06:34]

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.

[not at google - send to devlin, 2013/05/31 16:35:00]

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.

[cduvall, 2013/06/05 03:05:56]

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.

[not at google - send to devlin, 2013/06/05 19:03:44]

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.

[cduvall, 2013/06/05 23:27:59]

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

+  ChromeV8Context* context = *v8_context_set().GetAll().begin();
+  context->v8_context()->Enter();
+  RegisterSchemaGeneratedBindings(context->module_system(), context);
+  context->v8_context()->Exit();

[not at google - send to devlin, 2013/05/31 16:35:00]

use v8::Context::Scope here, and move into RegisterSchemaGeneratedBindings (may
also need a HandleScope there).

[cduvall, 2013/06/05 03:05:56]

Done.

+}
+
 void Dispatcher::OnUpdatePermissions(int reason_id,
                                      const std::string& extension_id,
                                      const APIPermissionSet& apis,
@@ -1281,6 +1322,7 @@ void Dispatcher::OnUpdatePermissions(int reason_id,
 
   PermissionsData::SetActivePermissions(extension, new_active);
   AddOrRemoveOriginPermissions(reason, extension, explicit_hosts);
+  RefreshSchemaGeneratedBindings();
 }
 
 void Dispatcher::OnUpdateTabSpecificPermissions(