Formalise a CHECK for the NULL NavigationEntry in ScriptBadgeController, and

add debugging for it.


BUG=138323


Review URL: https://chromiumcodereview.appspot.com/10826141

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150303 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/api/tabs/execute_code_in_tab_function.cc

@@ -100,25 +100,23 @@ bool ExecuteCodeInTabFunction::RunImpl() {
   return true;
 }
 
-void ExecuteCodeInTabFunction::OnExecuteCodeFinished(bool success,
-                                                     int32 page_id,
-                                                     const std::string& error,
+void ExecuteCodeInTabFunction::OnExecuteCodeFinished(const std::string& error,
+                                                     int32 on_page_id,
+                                                     const GURL& on_url,
                                                      const ListValue& result) {
-  if (!error.empty()) {
-    CHECK(!success);
+  if (!error.empty())
     SetError(error);
-  }
 
-  SendResponse(success);
+  SendResponse(error.empty());
 }
 
-void TabsExecuteScriptFunction::OnExecuteCodeFinished(bool success,
-                                                      int32 page_id,
-                                                      const std::string& error,
+void TabsExecuteScriptFunction::OnExecuteCodeFinished(const std::string& error,
+                                                      int32 on_page_id,
+                                                      const GURL& on_url,
                                                       const ListValue& result) {
   if (error.empty())
     SetResult(result.DeepCopy());
-  ExecuteCodeInTabFunction::OnExecuteCodeFinished(success, page_id, error,
+  ExecuteCodeInTabFunction::OnExecuteCodeFinished(error, on_page_id, on_url,
                                                   result);
 }
 

chrome/browser/extensions/api/tabs/execute_code_in_tab_function.h

@@ -32,9 +32,9 @@ class ExecuteCodeInTabFunction : public AsyncExtensionFunction {
   virtual bool RunImpl() OVERRIDE;
 
   // Message handler.
-  virtual void OnExecuteCodeFinished(bool success,
-                                     int32 page_id,
-                                     const std::string& error,
+  virtual void OnExecuteCodeFinished(const std::string& error,
+                                     int32 on_page_id,
+                                     const GURL& on_url,
                                      const ListValue& script_result);
 
  private:
@@ -76,8 +76,9 @@ class TabsExecuteScriptFunction : public ExecuteCodeInTabFunction {
  private:
   virtual ~TabsExecuteScriptFunction() {}
 
-  virtual void OnExecuteCodeFinished(bool success, int32 page_id,
-                                     const std::string& error,
+  virtual void OnExecuteCodeFinished(const std::string& error,
+                                     int32 on_page_id,
+                                     const GURL& on_url,
                                      const ListValue& script_result) OVERRIDE;
 
   DECLARE_EXTENSION_FUNCTION_NAME("tabs.executeScript")

chrome/browser/extensions/api/tabs/tabs.cc

@@ -1347,18 +1347,15 @@ void UpdateTabFunction::PopulateResult() {
   }
 }
 
-void UpdateTabFunction::OnExecuteCodeFinished(bool success,
-                                              int32 page_id,
-                                              const std::string& error,
+void UpdateTabFunction::OnExecuteCodeFinished(const std::string& error,
+                                              int32 on_page_id,
+                                              const GURL& url,
                                               const ListValue& script_result) {
-  if (!error.empty()) {
-    CHECK(!success);
-    error_ = error;
-  }
-
-  if (success)
+  if (error.empty())
     PopulateResult();
-  SendResponse(success);
+  else
+    error_ = error;
+  SendResponse(error.empty());
 }
 
 bool MoveTabsFunction::RunImpl() {

chrome/browser/extensions/api/tabs/tabs.h

@@ -129,9 +129,9 @@ class UpdateTabFunction : public AsyncExtensionFunction {
 
  private:
   virtual bool RunImpl() OVERRIDE;
-  void OnExecuteCodeFinished(bool success,
-                             int32 page_id,
-                             const std::string& error,
+  void OnExecuteCodeFinished(const std::string& error,
+                             int32 on_page_id,
+                             const GURL& on_url,
                              const ListValue& script_result);
 
   DECLARE_EXTENSION_FUNCTION_NAME("tabs.update")

chrome/browser/extensions/script_badge_controller.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/extensions/script_badge_controller.h"
 
+#include "base/logging.h"
+#include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "chrome/browser/extensions/browser_event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -20,6 +23,7 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
+#include "googleurl/src/gurl.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
 
@@ -103,11 +107,27 @@ LocationBarController::Action ScriptBadgeController::OnClicked(
 
 void ScriptBadgeController::OnExecuteScriptFinished(
     const std::string& extension_id,
-    bool success,
-    int32 page_id,
     const std::string& error,
+    int32 on_page_id,
+    const GURL& on_url,
     const base::ListValue& script_results) {
-  if (success && page_id == GetPageID()) {
+  int32 current_page_id = GetPageID();
+
+  // Tracking down http://crbug.com/138323.
+  if (current_page_id < 0) {
+    std::string message = base::StringPrintf(
+        "Expected a page ID of %d on URL \"%s\", but there was no navigation "
+        "entry. Current URL is \"%s\", extension ID is %s.",
+        on_page_id,
+        on_url.spec().c_str(),
+        tab_contents_->web_contents()->GetURL().spec().c_str(),
+        extension_id.c_str());
+    char buf[1024];
+    base::snprintf(buf, arraysize(buf), "%s", message.c_str());
+    CHECK(false) << message;
+  }
+
+  if (error.empty() && on_page_id == current_page_id) {
     if (MarkExtensionExecuting(extension_id))
       NotifyChange();
   }
@@ -119,8 +139,9 @@ ExtensionService* ScriptBadgeController::GetExtensionService() {
 }
 
 int32 ScriptBadgeController::GetPageID() {
-  return tab_contents_->web_contents()->GetController().GetActiveEntry()->
-      GetPageID();
+  content::NavigationEntry* nav_entry =
+      tab_contents_->web_contents()->GetController().GetActiveEntry();
+  return nav_entry ? nav_entry->GetPageID() : -1;
 }
 
 void ScriptBadgeController::NotifyChange() {
@@ -160,9 +181,34 @@ bool ScriptBadgeController::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
+namespace {
+std::string JoinExtensionIDs(const std::set<std::string>& ids) {
+  std::vector<std::string> as_vector(ids.begin(), ids.end());
+  return "[" + JoinString(as_vector, ',') + "]";
+}
+}  // namespace
+
 void ScriptBadgeController::OnContentScriptsExecuting(
-    const std::set<std::string>& extension_ids, int32 page_id) {
-  if (page_id != GetPageID())
+    const std::set<std::string>& extension_ids,
+    int32 on_page_id,
+    const GURL& on_url) {
+  int32 current_page_id = GetPageID();
+
+  // Tracking down http://crbug.com/138323.
+  if (current_page_id < 0) {
+    std::string message = base::StringPrintf(
+        "Expected a page ID of %d on URL \"%s\", but there was no navigation "
+        "entry. Current URL is \"%s\", extension IDs are %s.",
+        on_page_id,
+        on_url.spec().c_str(),
+        tab_contents_->web_contents()->GetURL().spec().c_str(),
+        JoinExtensionIDs(extension_ids).c_str());
+    char buf[1024];
+    base::snprintf(buf, arraysize(buf), "%s", message.c_str());
+    CHECK(false) << message;
+  }
+
+  if (on_page_id != current_page_id)
     return;
 
   bool changed = false;

chrome/browser/extensions/script_badge_controller.h

@@ -20,6 +20,7 @@
 
 class ExtensionAction;
 class ExtensionService;
+class GURL;
 class TabContents;
 
 namespace base {
@@ -65,14 +66,17 @@ class ScriptBadgeController
 
   // ScriptExecutor::Observer implementation.
   virtual void OnExecuteScriptFinished(
-      const std::string& extension_id, bool success, int32 page_id,
-      const std::string& error, const base::ListValue& script_result) OVERRIDE;
+      const std::string& extension_id,
+      const std::string& error,
+      int32 on_page_id,
+      const GURL& on_url,
+      const base::ListValue& script_result) OVERRIDE;
 
  private:
   // Gets the ExtensionService for |tab_contents_|.
   ExtensionService* GetExtensionService();
 
-  // Gets the current page ID.
+  // Gets the current page ID, or -1 if no navigation entry has been committed.
   int32 GetPageID();
 
   // content::WebContentsObserver implementation.
@@ -88,7 +92,8 @@ class ScriptBadgeController
 
   // IPC::Message handlers.
   void OnContentScriptsExecuting(const std::set<std::string>& extension_ids,
-                                 int32 page_id);
+                                 int32 page_id,
+                                 const GURL& on_url);
 
   // Adds the extension's icon to the list of script badges.  Returns
   // the script badge ExtensionAction that was added, or NULL if

chrome/browser/extensions/script_badge_controller_unittest.cc

@@ -120,10 +120,11 @@ TEST_F(ScriptBadgeControllerTest, ExecutionMakesBadgeVisible) {
 
   ListValue val;
   script_badge_controller_->OnExecuteScriptFinished(
-      extension->id(), true,
+      extension->id(),
+      "",  // no error
       tab_contents()->web_contents()->GetController().GetActiveEntry()->
       GetPageID(),
-      "",
+      GURL(""),
       val);
   EXPECT_THAT(script_badge_controller_->GetCurrentActions(),
               testing::ElementsAre(extension->script_badge()));
@@ -147,10 +148,11 @@ TEST_F(ScriptBadgeControllerTest, FragmentNavigation) {
 
     ListValue val;
     script_badge_controller_->OnExecuteScriptFinished(
-        extension->id(), true,
+        extension->id(),
+        "",  // no error
         tab_contents()->web_contents()->GetController().GetActiveEntry()->
             GetPageID(),
-        "",
+        GURL(""),
         val);
 
     EXPECT_THAT(script_badge_controller_->GetCurrentActions(),

chrome/browser/extensions/script_executor.cc

@@ -66,23 +66,24 @@ class Handler : public content::WebContentsObserver {
 
   virtual void WebContentsDestroyed(content::WebContents* tab) OVERRIDE {
     base::ListValue val;
-    callback_.Run(false, -1, kRendererDestroyed, val);
+    callback_.Run(kRendererDestroyed, -1, GURL(""), val);
     delete this;
   }
 
  private:
   void OnExecuteCodeFinished(int request_id,
-                             bool success,
-                             int32 page_id,
                              const std::string& error,
+                             int32 on_page_id,
+                             const GURL& on_url,
                              const base::ListValue& script_result) {
     if (observer_list_) {
       FOR_EACH_OBSERVER(ScriptExecutor::Observer, *observer_list_,
-                        OnExecuteScriptFinished(extension_id_, success,
-                                                page_id, error, script_result));
+                        OnExecuteScriptFinished(extension_id_, error,
+                                                on_page_id, on_url,
+                                                script_result));
     }
 
-    callback_.Run(success, page_id, error, script_result);
+    callback_.Run(error, on_page_id, on_url, script_result);
     delete this;
   }
 

chrome/browser/extensions/script_executor.h

@@ -11,6 +11,8 @@
 #include "base/observer_list.h"
 #include "chrome/common/extensions/user_script.h"
 
+class GURL;
+
 namespace base {
 class ListValue;
 }  // namespace base
@@ -48,9 +50,9 @@ class ScriptExecutor {
     ISOLATED_WORLD,
   };
 
-  // Callback from ExecuteScript. The arguments are (success, page_id, error,
-  // result). page_id is only valid on success, error is only valid on !success.
-  typedef base::Callback<void(bool, int32, const std::string&,
+  // Callback from ExecuteScript. The arguments are (error, on_page_id, on_url,
+  // result). Success is implied by an empty error.
+  typedef base::Callback<void(const std::string&, int32, const GURL&,
                               const base::ListValue&)>
       ExecuteScriptCallback;
 
@@ -62,9 +64,9 @@ class ScriptExecutor {
     virtual ~Observer();
 
     virtual void OnExecuteScriptFinished(const std::string& extension_id,
-                                         bool success,
-                                         int32 page_id,
                                          const std::string& error,
+                                         int32 on_page_id,
+                                         const GURL& on_url,
                                          const base::ListValue&) = 0;
    private:
     ScriptExecutor& script_executor_;

chrome/common/extensions/extension_messages.h

@@ -14,6 +14,7 @@
 #include "chrome/common/view_type.h"
 #include "chrome/common/web_apps.h"
 #include "content/public/common/common_param_traits.h"
+#include "googleurl/src/gurl.h"
 #include "ipc/ipc_message_macros.h"
 
 #define IPC_MESSAGE_START ExtensionMsgStart
@@ -431,9 +432,9 @@ IPC_SYNC_MESSAGE_CONTROL1_1(ExtensionHostMsg_GetMessageBundle,
 // Sent from the renderer to the browser to return the script running result.
 IPC_MESSAGE_ROUTED5(ExtensionHostMsg_ExecuteCodeFinished,
                     int /* request id */,
-                    bool /* whether the script ran successfully */,
+                    std::string /* error; empty implies success */,
                     int32 /* page_id the code executed on, if successful */,
-                    std::string /* error message, if unsuccessful */,
+                    GURL /* URL of the code executed on, if successful */,
                     ListValue /* result of the script */)
 
 // Sent from the renderer to the browser to notify that content scripts are
@@ -441,9 +442,10 @@ IPC_MESSAGE_ROUTED5(ExtensionHostMsg_ExecuteCodeFinished,
 // Note that the page_id is for the parent (or more accurately the topmost)
 // frame (e.g. if executing in an iframe this is the page ID of the parent,
 // unless the parent is an iframe... etc).
-IPC_MESSAGE_ROUTED2(ExtensionHostMsg_ContentScriptsExecuting,
+IPC_MESSAGE_ROUTED3(ExtensionHostMsg_ContentScriptsExecuting,
                     std::set<std::string> /* extensions that have scripts */,
-                    int32 /* page_id of the _topmost_ frame */)
+                    int32 /* page_id of the _topmost_ frame */,
+                    GURL /* url of the _topmost_ frame */)
 
 IPC_MESSAGE_ROUTED2(ExtensionHostMsg_DidGetApplicationInfo,
                     int32 /* page_id */,

chrome/renderer/extensions/extension_helper.cc

@@ -345,7 +345,7 @@ void ExtensionHelper::OnExecuteCode(
   if (!main_frame) {
     ListValue val;
     Send(new ExtensionHostMsg_ExecuteCodeFinished(
-        routing_id(), params.request_id, false, -1, "", val));
+        routing_id(), params.request_id, "No main frame", -1, GURL(""), val));
     return;
   }
 

chrome/renderer/extensions/user_script_scheduler.cc

@@ -145,7 +145,11 @@ void UserScriptScheduler::ExecuteCodeImpl(
   // be out of sync. We just ignore this situation.
   if (!extension) {
     render_view->Send(new ExtensionHostMsg_ExecuteCodeFinished(
-        render_view->GetRoutingID(), params.request_id, true, -1, "",
+        render_view->GetRoutingID(),
+        params.request_id,
+        "",  // no error
+        -1,
+        GURL(""),
         execution_results));
     return;
   }
@@ -179,11 +183,11 @@ void UserScriptScheduler::ExecuteCodeImpl(
           render_view->Send(new ExtensionHostMsg_ExecuteCodeFinished(
               render_view->GetRoutingID(),
               params.request_id,
-              false,
-              -1,
               ExtensionErrorUtils::FormatErrorMessage(
                   extension_manifest_errors::kCannotAccessPage,
                   frame->document().url().spec()),
+              -1,
+              GURL(""),
               execution_results));
           return;
         }
@@ -232,9 +236,9 @@ void UserScriptScheduler::ExecuteCodeImpl(
   render_view->Send(new ExtensionHostMsg_ExecuteCodeFinished(
       render_view->GetRoutingID(),
       params.request_id,
-      true,
+      "",  // no error
       render_view->GetPageId(),
-      "",
+      UserScriptSlave::GetDataSourceURLForFrame(frame_),
       execution_results));
 }
 

chrome/renderer/extensions/user_script_slave.cc

@@ -339,16 +339,14 @@ void UserScriptSlave::InjectScripts(WebFrame* frame,
 
   // Notify the browser if any extensions are now executing scripts.
   if (!extensions_executing_scripts.empty()) {
-    WebKit::WebFrame* target_frame = frame;
-    while (target_frame->parent())
-      target_frame = target_frame->parent();
-
+    WebKit::WebFrame* top_frame = frame->top();
     content::RenderView* render_view =
-        content::RenderView::FromWebView(target_frame->view());
+        content::RenderView::FromWebView(top_frame->view());
     render_view->Send(new ExtensionHostMsg_ContentScriptsExecuting(
         render_view->GetRoutingID(),
         extensions_executing_scripts,
-        render_view->GetPageId()));
+        render_view->GetPageId(),
+        GetDataSourceURLForFrame(top_frame)));
   }
 
   // Log debug info.