Merge 216743 "Don't override extension bindings that already exi..."

> Don't override extension bindings that already exist, it causes event listeners
> to be lost.
> 
> BUG=269149
> R=mpcomplete@chromium.org
> 
> Review URL: https://codereview.chromium.org/22588004

TBR=kalman@chromium.org

Review URL: https://codereview.chromium.org/22916002

git-svn-id: svn://svn.chromium.org/chrome/branches/1547/src@217043 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 102 insertions, 4 deletions

diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index c2ec75b..ff31ca4 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -514,6 +514,8 @@ bool ExtensionBrowserTest::WaitForExtensionViewsToLoad() {
   content::NotificationRegistrar registrar;
   registrar.Add(this, content::NOTIFICATION_LOAD_STOP,
                 content::NotificationService::AllSources());
+  registrar.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                content::NotificationService::AllSources());
 
   ExtensionProcessManager* manager =
       extensions::ExtensionSystem::Get(profile())->process_manager();
@@ -731,7 +733,8 @@ void ExtensionBrowserTest::Observe(
     }
 
     case content::NOTIFICATION_LOAD_STOP:
-      VLOG(1) << "Got LOAD_STOP notification.";
+    case content::NOTIFICATION_WEB_CONTENTS_DESTROYED:
+      VLOG(1) << "Got LOAD_STOP or WEB_CONTENTS_DESTROYED notification.";
       base::MessageLoopForUI::current()->Quit();
       break;
 
diff --git a/chrome/common/extensions/api/test.json b/chrome/common/extensions/api/test.json
index a06052d..94742b5 100644
--- a/chrome/common/extensions/api/test.json
+++ b/chrome/common/extensions/api/test.json
@@ -218,6 +218,22 @@
         ]
       },
       {
+        "name": "assertThrows",
+        "type": "function",
+        "nocompile": true,
+        "parameters": [
+          {"type": "function", "name": "fn"},
+          {
+            "type": "object",
+            "name": "self",
+            "additionalProperties": {"type": "any"},
+            "optional": true
+          },
+          {"type": "array", "items": {"type": "any"}, "name": "args"},
+          {"type": "string", "name": "message", "optional": true}
+        ]
+      },
+      {
         "name": "callback",
         "type": "function",
         "nocompile": true,
diff --git a/chrome/renderer/extensions/dispatcher.cc b/chrome/renderer/extensions/dispatcher.cc
index 1757597..75c2fe5 100644
--- a/chrome/renderer/extensions/dispatcher.cc
+++ b/chrome/renderer/extensions/dispatcher.cc
@@ -795,9 +795,29 @@ void Dispatcher::RegisterBinding(const std::string& api_name,
   std::string bind_name;
   v8::Handle<v8::Object> bind_object =
       GetOrCreateBindObjectIfAvailable(api_name, &bind_name, context);
+
+  // Empty if the bind object failed to be created, probably because the
+  // extension overrode chrome with a non-object, e.g. window.chrome = true.
   if (bind_object.IsEmpty())
     return;
 
+  v8::Local<v8::String> v8_api_name = v8::String::New(api_name.c_str());
+  if (bind_object->HasRealNamedProperty(v8_api_name)) {
+    // The bind object may already have the property if the API has been
+    // registered before (or if the extension has put something there already,
+    // but, whatevs).
+    //
+    // In the former case, we need to re-register the bindings for the APIs
+    // which the extension now has permissions for (if any), but not touch any
+    // others so that we don't destroy state such as event listeners.
+    //
+    // TODO(kalman): Only register available APIs to make this all moot.
+    if (bind_object->HasRealNamedCallbackProperty(v8_api_name))
+      return;  // lazy binding still there, nothing to do
+    if (bind_object->Get(v8_api_name)->IsObject())
+      return;  // binding has already been fully installed
+  }
+
   ModuleSystem* module_system = context->module_system();
   if (lazy_bindings_map_.find(api_name) != lazy_bindings_map_.end()) {
     InstallBindings(module_system, context->v8_context(), api_name);
diff --git a/chrome/renderer/resources/extensions/test_custom_bindings.js b/chrome/renderer/resources/extensions/test_custom_bindings.js
index 231e339..fe330d3 100644
--- a/chrome/renderer/resources/extensions/test_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/test_custom_bindings.js
@@ -228,6 +228,18 @@ binding.registerCustomHook(function(api) {
     chromeTest.assertEq(expectedError, chrome.runtime.lastError.message);
   });
 
+  apiFunctions.setHandleRequest('assertThrows',
+                                function(fn, self, args, message) {
+    assertTrue(typeof fn == 'function');
+    try {
+      fn.apply(self, args);
+      chromeTest.fail('Did not throw error: ' + fn);
+    } catch (e) {
+      if (message !== undefined)
+        chromeTest.assertEq(message, e.message);
+    }
+  });
+
   function safeFunctionApply(func, args) {
     try {
       if (func)
diff --git a/chrome/test/data/extensions/api_test/bindings/event_overriding/background.js b/chrome/test/data/extensions/api_test/bindings/event_overriding/background.js
new file mode 100644
index 0000000..9ac00c4
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/bindings/event_overriding/background.js
@@ -0,0 +1,27 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function test() {
+  var testWindowId = null;
+
+  chrome.windows.create({url: 'page.html'}, function(w) {
+    testWindowId = w.id;
+    chrome.windows.remove(w.id);
+  });
+
+  chrome.windows.onRemoved.addListener(function listener(windowId) {
+    if (windowId != testWindowId)
+      return;  // I guess some other window might have closed?
+
+    // If the event hasn't been overridden the count will be 1.
+    chrome.test.assertEq(1, chrome.windows.onRemoved.getListenerCount());
+
+    // This used to crash since we try to register the event more than once.
+    chrome.windows.onRemoved.removeListener(listener);
+    chrome.windows.onRemoved.addListener(listener);
+    chrome.test.succeed();
+  });
+}
+
+chrome.test.runTests([test]);
diff --git a/chrome/test/data/extensions/api_test/bindings/event_overriding/manifest.json b/chrome/test/data/extensions/api_test/bindings/event_overriding/manifest.json
new file mode 100644
index 0000000..978bd7c
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/bindings/event_overriding/manifest.json
@@ -0,0 +1,9 @@
+{
+  "name": "bindings/event_overriding",
+  "version": "1",
+  "manifest_version": 2,
+  "background": {
+    "scripts": ["background.js"],
+    "persistent": false
+  }
+}
diff --git a/chrome/test/data/extensions/api_test/bindings/event_overriding/page.html b/chrome/test/data/extensions/api_test/bindings/event_overriding/page.html
new file mode 100644
index 0000000..dd5ba89
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/bindings/event_overriding/page.html
@@ -0,0 +1 @@
+Some page for the test to open.
diff --git a/chrome/test/data/extensions/api_test/permissions/optional/background.js b/chrome/test/data/extensions/api_test/permissions/optional/background.js
index 9be32d6..8285e28 100644
--- a/chrome/test/data/extensions/api_test/permissions/optional/background.js
+++ b/chrome/test/data/extensions/api_test/permissions/optional/background.js
@@ -5,6 +5,7 @@
 var assertEq = chrome.test.assertEq;
 var assertFalse = chrome.test.assertFalse;
 var assertTrue = chrome.test.assertTrue;
+var assertThrows = chrome.test.assertThrows;
 var fail = chrome.test.callbackFail;
 var pass = chrome.test.callbackPass;
 var listenOnce = chrome.test.listenOnce;
@@ -193,8 +194,13 @@ chrome.test.getConfig(function(config) {
             chrome.permissions.getAll(pass(function(permissions) {
               assertTrue(checkPermSetsEq(initialPermissions, permissions));
             }));
-            assertEq(undefined, chrome.bookmarks);
-      }));
+            assertTrue(typeof chrome.bookmarks == 'object' &&
+                       chrome.bookmarks != null);
+            assertThrows(
+              chrome.bookmarks.getTree, [function(){}],
+              "'bookmarks' requires a different Feature that is not present.");
+          }
+      ));
     },
 
     // The user shouldn't have to approve permissions that have no warnings.
@@ -287,7 +293,11 @@ chrome.test.getConfig(function(config) {
       });
       listenOnce(chrome.permissions.onRemoved,
                  function(permissions) {
-        assertEq(undefined, chrome.bookmarks);
+        assertTrue(typeof chrome.bookmarks == 'object' &&
+                   chrome.bookmarks != null);
+        assertThrows(
+          chrome.bookmarks.getTree, [function(){}],
+          "'bookmarks' requires a different Feature that is not present.");
       });
 
       chrome.permissions.request(