Do not pass URLs in onUpdated events to extensions unless they have the

chrome/browser/extensions/browser_event_router.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/extensions/browser_event_router.h"
 
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
 #include "chrome/browser/extensions/api/tabs/tabs_constants.h"
@@ skipping 379 matching lines @@
     return;
 
   scoped_ptr<ListValue> args(new ListValue());
   args->Append(Value::CreateIntegerValue(window_id));
 
   DispatchEvent(profile, event_name, args.Pass(),
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
 static void WillDispatchTabUpdatedEvent(WebContents* contents,
+                                        DictionaryValue* changed_properties,
                                         Profile* profile,
                                         const Extension* extension,
                                         ListValue* event_args) {
+  // Overwrite the second argument with the appropriate properties dictionary,
+  // depending on extension permissions.
+  DictionaryValue* properties_value =
+      ExtensionTabUtil::ScrubTabValue(contents, extension, changed_properties);

[not at google - send to devlin, 2013/01/08 21:25:09]

since DispatchTabUpdatedEvent is taking ownership of changed_properties it seems
like we can save a bit of work and take ownership here, and avoid copying the
value. 

in which case, take a scoped_ptr<DictionaryValue> for changed_properties

[mvrable, 2013/01/09 01:46:59]

I believe the way this callback works is that it runs once before an event is
sent to an extension.  If there are multiple extensions listening for the event,
the callback will run multiple times--so we can't free changed_properties here.

We could potentially reduce memory allocations by having a sanitized and
non-sanitized version of changed_properties and selecting the right one at each
step, but that adds extra complications and probably isn't worth it (especially
as tab_value is already being recreated on each call to
WillDispatchTabUpdatedEvent anyway).

+  event_args->Set(1, properties_value);
+
+  // Overwrite the third arg with our tab value as seen by this extension.
   DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
       contents, extension);
-  // Overwrite the third arg with our tab value as seen by this extension.
   event_args->Set(2, tab_value);
 }
 
 void BrowserEventRouter::DispatchTabUpdatedEvent(
     WebContents* contents, DictionaryValue* changed_properties) {

[not at google - send to devlin, 2013/01/08 21:25:09]

cleanup: make this take a scoped_ptr<DictionaryValue> to indicate ownership

[mvrable, 2013/01/09 01:46:59]

Done.

   DCHECK(changed_properties);
   DCHECK(contents);
 
   // The state of the tab (as seen from the extension point of view) has
   // changed.  Send a notification to the extension.
   scoped_ptr<ListValue> args_base(new ListValue());
 
+  // Ownership of changed_properties has been passed to us; ensure it is
+  // cleaned up when we return.
+  scoped_ptr<DictionaryValue> changed_properties_deleter(changed_properties);
+
   // First arg: The id of the tab that changed.
   args_base->AppendInteger(ExtensionTabUtil::GetTabId(contents));
 
-  // Second arg: An object containing the changes to the tab state.
-  args_base->Append(changed_properties);
+  // Second arg: An object containing the changes to the tab state.  Filled in
+  // by WillDispatchTabUpdatedEvent as a copy of changed_properties, if the
+  // extension has the tabs permission.
 
   // Third arg: An object containing the state of the tab. Filled in by
   // WillDispatchTabUpdatedEvent.
   Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
 
   scoped_ptr<Event> event(new Event(events::kOnTabUpdated, args_base.Pass()));
   event->restrict_to_profile = profile;
   event->user_gesture = EventRouter::USER_GESTURE_NOT_ENABLED;
   event->will_dispatch_callback =
-      base::Bind(&WillDispatchTabUpdatedEvent, contents);
+      base::Bind(&WillDispatchTabUpdatedEvent, contents, changed_properties);

[not at google - send to devlin, 2013/01/08 21:25:09]

changed_properties will have been deleted by the time the callback runs;
changing WillDispatchTabUpdatedEvent is to take a scoped_ptr might work, I can't
remember, maybe you need to use base::Owned in bind_helpers.h

[mvrable, 2013/01/09 01:46:59]

I believe that the callback is run from within BroadcastEvent below, and thus
will fall within the lifetime of the changed_properties object.

   ExtensionSystem::Get(profile)->event_router()->BroadcastEvent(event.Pass());
 }
 
 BrowserEventRouter::TabEntry* BrowserEventRouter::GetTabEntry(
     const WebContents* contents) {
   int tab_id = ExtensionTabUtil::GetTabId(contents);
   std::map<int, TabEntry>::iterator i = tab_entries_.find(tab_id);
   if (tab_entries_.end() == i)
     return NULL;
   return &i->second;
@@ skipping 134 matching lines @@
 
     DispatchEventToExtension(profile,
                              extension_action.extension_id(),
                              event_name,
                              args.Pass(),
                              EventRouter::USER_GESTURE_ENABLED);
   }
 }
 
 }  // namespace extensions