Make ScriptBadgeController record when extensions run scripts on pages yet to

chrome/browser/extensions/script_badge_controller.h

Index: chrome/browser/extensions/script_badge_controller.h
diff --git a/chrome/browser/extensions/script_badge_controller.h b/chrome/browser/extensions/script_badge_controller.h
index 61001bd97c1f1739e9286482a346a845a20b77d7..fddba4621315c1863309f9f80ddd7940642fc501 100644
--- a/chrome/browser/extensions/script_badge_controller.h
+++ b/chrome/browser/extensions/script_badge_controller.h
@@ -64,15 +64,21 @@ class ScriptBadgeController
   virtual void NotifyChange() OVERRIDE;
 
   // ScriptExecutor::Observer implementation.
+  // Public to be called from tests.
   virtual void OnExecuteScriptFinished(
       const std::string& extension_id, bool success, int32 page_id,
       const std::string& error, const base::ListValue& script_result) OVERRIDE;
 
+  // IPC::Message handlers.
+  // Public to be called from tests.

[Jeffrey Yasskin, 2012/07/25 22:22:39]

<obligatory-c++-trivia>
Because these implement public methods from public base classes, you can
actually call them from tests using
"static_cast<content::WebContentsObserver*>(script_badge_controller)->OnContentScriptsExecuting(...)".
That defeats the general point of making them private, although it might serve
enough documentation purpose to be worth privatizing the methods anyway.
</obligatory-c++-trivia>

[not at google - send to devlin, 2012/07/26 08:52:46]

I was going to say "I like it" and started changing it.

Then I was like, damn it past-Ben, you forgot to make that method virtual, lazy.

Then I realised it's not an override, just one of those methods that you
delegate IPC handling to. So, public it is :(

(either that or faking out IPCs, but tooo much effort).

+  void OnContentScriptsExecuting(const std::set<std::string>& extension_ids,
+                                 int32 page_id);
+
  private:
   // Gets the ExtensionService for |tab_contents_|.
   ExtensionService* GetExtensionService();
 
-  // Gets the current page ID.
+  // Gets the current page ID, or -1 if there isn't one yet.
   int32 GetPageID();
 
   // content::WebContentsObserver implementation.
@@ -86,10 +92,6 @@ class ScriptBadgeController
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // IPC::Message handlers.
-  void OnContentScriptsExecuting(const std::set<std::string>& extension_ids,
-                                 int32 page_id);
-
   // Adds the extension's icon to the list of script badges.  Returns
   // the script badge ExtensionAction that was added, or NULL if
   // extension_id isn't valid.
@@ -115,6 +117,10 @@ class ScriptBadgeController
   // The extensions that have actions in current_actions_.
   std::set<std::string> extensions_in_current_actions_;
 
+  // Extensions that have actions in a page ID that hasn't been reached yet.

[Jeffrey Yasskin, 2012/07/25 22:22:39]

This feels like something that will bite us later if we don't figure out what's
really causing it. How does the OnExecuting message get received before the
navigation entry is set?

[not at google - send to devlin, 2012/07/26 08:52:46]

Yeah, so the crash was from OnContentScriptExecuting.

My initial assumption, and the assumption of this patch, is that it runs early
(like on document creation) then send up the IPC, somehow before the navigation
entry is set.

The documentation in NavigationController just says "it can be NULL" but not
why. I assumed it was before there were any navigation entries? And if these
IPCs can arrive before the navigation entry is created, then presumably they can
arrive when they're stale too?

I briefly also wondered if it was because the navigation stuff had been torn
down, but then why would the IPC arrive? Saying that now, I realise it's just as
feasible as it not being around when it's been created, so eh.

I guess I should really go and look through the content code to verify all this.
It can't be a black box forever.

+  typedef std::map<int, std::vector<std::string> > FutureActionsMap;
+  FutureActionsMap future_actions_;
+
   // Listen to extension unloaded notifications.
   content::NotificationRegistrar registrar_;