The DOM Automation controller object uses the RenderView instance as the message sender, i.e. to send replies to javascript requests issued by the browser.

The DOM automation controller object is bound to the frame in the WindowObjectcleared code path.The current implementation maintains the message sender object as a static pointer, which causes a crash if the RenderView instance goes out of scope. 

This can be reproduced with a page which causes a popup window to show up and close. If we attempt to use the Dom Automation controller instance bound to the original Renderview, which is still valid, we crash.

The fix is to maintain the message sender as a member variable. The lifetime of the Dom Automation controller instance depends on the RenderView lifetime anyway as it is a member variable. This mimics the other CppBindings like the external host bindings, etc.

Added an automation test to test this case. I verified that the test crashes without this fix.

This fixes bug http://code.google.com/p/chromium/issues/detail?id=3941

Bug=3941

Review URL: http://codereview.chromium.org/21441

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

4 files changed, 83 insertions, 18 deletions

diff --git a/chrome/renderer/automation/dom_automation_controller.cc b/chrome/renderer/automation/dom_automation_controller.cc
index 56aea6e..0c320ca 100644
--- a/chrome/renderer/automation/dom_automation_controller.cc
+++ b/chrome/renderer/automation/dom_automation_controller.cc
@@ -8,16 +8,15 @@
 #include "chrome/common/render_messages.h"
 #include "base/string_util.h"
 
-IPC::Message::Sender* DomAutomationController::sender_(NULL);
-int DomAutomationController::routing_id_(MSG_ROUTING_NONE);
-int DomAutomationController::automation_id_(MSG_ROUTING_NONE);
-
-DomAutomationController::DomAutomationController(){
-  BindMethod("send", &DomAutomationController::send);
-  BindMethod("setAutomationId", &DomAutomationController::setAutomationId);
+DomAutomationController::DomAutomationController()
+    : sender_(NULL),
+      routing_id_(MSG_ROUTING_NONE),
+      automation_id_(MSG_ROUTING_NONE) {
+  BindMethod("send", &DomAutomationController::Send);
+  BindMethod("setAutomationId", &DomAutomationController::SetAutomationId);
 }
 
-void DomAutomationController::send(const CppArgumentList& args,
+void DomAutomationController::Send(const CppArgumentList& args,
                                    CppVariant* result) {
   if (args.size() != 1) {
     result->SetNull();
@@ -29,6 +28,12 @@ void DomAutomationController::send(const CppArgumentList& args,
     return;
   }
 
+  if (!sender_) {
+    NOTREACHED();
+    result->SetNull();
+    return;
+  }
+
   std::string json;
   JSONStringValueSerializer serializer(&json);
   scoped_ptr<Value> value;
@@ -81,7 +86,7 @@ void DomAutomationController::send(const CppArgumentList& args,
   return;
 }
 
-void DomAutomationController::setAutomationId(
+void DomAutomationController::SetAutomationId(
     const CppArgumentList& args, CppVariant* result) {
   if (args.size() != 1) {
     result->SetNull();
diff --git a/chrome/renderer/automation/dom_automation_controller.h b/chrome/renderer/automation/dom_automation_controller.h
index 3856b3c..552adab 100644
--- a/chrome/renderer/automation/dom_automation_controller.h
+++ b/chrome/renderer/automation/dom_automation_controller.h
@@ -76,33 +76,32 @@
 class DomAutomationController : public CppBoundClass {
  public:
   DomAutomationController();
-  ~DomAutomationController() {}
 
   // Makes the renderer send a javascript value to the app.
   // The value to be sent can be either of type NPString,
   // Number (double casted to int32) or boolean.
   // The function returns true/false based on the result of actual send over
   // IPC. It sets the return value to null on unexpected errors or arguments.
-  void send(const CppArgumentList& args, CppVariant* result);
+  void Send(const CppArgumentList& args, CppVariant* result);
 
-  void setAutomationId(const CppArgumentList& args, CppVariant* result);
+  void SetAutomationId(const CppArgumentList& args, CppVariant* result);
 
   // TODO(vibhor): Implement later
   // static CppBindingObjectMethod sendLater;
   // static CppBindingObjectMethod sendNow;
 
-  static void set_routing_id(int routing_id) { routing_id_ = routing_id; }
+  void set_routing_id(int routing_id) { routing_id_ = routing_id; }
 
-  static void set_message_sender(IPC::Message::Sender* sender) {
+  void set_message_sender(IPC::Message::Sender* sender) {
     sender_ = sender;
   }
 
  private:
-   static IPC::Message::Sender* sender_;
+   IPC::Message::Sender* sender_;
 
    // Refer to the comments at the top of the file for more details.
-   static int routing_id_;  // routing id to be used by first channel.
-   static int automation_id_;  // routing id to be used by the next channel.
+   int routing_id_;  // routing id to be used by first channel.
+   int automation_id_;  // routing id to be used by the next channel.
 };
 
 #endif  // CHROME_RENDERER_AUTOMATION_DOM_AUTOMATION_CONTROLLER_H__
diff --git a/chrome/test/automation/automation_proxy_uitest.cc b/chrome/test/automation/automation_proxy_uitest.cc
index acdf819..ee1d4db 100644
--- a/chrome/test/automation/automation_proxy_uitest.cc
+++ b/chrome/test/automation/automation_proxy_uitest.cc
@@ -907,4 +907,38 @@ TEST_F(AutomationProxyTest, DISABLED_AppModalDialogTest) {
   EXPECT_TRUE(tab->ExecuteAndExtractInt(
       L"", L"window.domAutomationController.send(result);", &result));
   EXPECT_EQ(1, result);
-}
\ No newline at end of file
+}
+
+class AutomationProxyTest5 : public UITest {
+ protected:
+  AutomationProxyTest5() {
+    show_window_ = true;
+    dom_automation_enabled_ = true;
+    // We need to disable popup blocking to ensure that the RenderView
+    // instance for the popup actually closes.
+    launch_arguments_.AppendSwitch(switches::kDisablePopupBlocking);
+  }
+};
+
+TEST_F(AutomationProxyTest5, TestLifetimeOfDomAutomationController) {
+  scoped_ptr<BrowserProxy> window(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(window.get());
+
+  scoped_ptr<TabProxy> tab(window->GetTab(0));
+  ASSERT_TRUE(tab.get());
+
+  std::wstring filename(test_data_directory_);
+  file_util::AppendToPath(&filename, L"dom_automation_test_with_popup.html");
+
+  tab->NavigateToURL(net::FilePathToFileURL(filename));
+
+  // Allow some time for the popup to show up and close.
+  Sleep(2000);
+
+  std::wstring expected(L"string");
+  std::wstring jscript = CreateJSString(L"\"" + expected + L"\"");
+  std::wstring actual;
+  ASSERT_TRUE(tab->ExecuteAndExtractString(L"", jscript, &actual));
+  ASSERT_STREQ(expected.c_str(), actual.c_str());
+}
+
diff --git a/chrome/test/data/dom_automation_test_with_popup.html b/chrome/test/data/dom_automation_test_with_popup.html
new file mode 100644
index 0000000..a19b5a0
--- /dev/null
+++ b/chrome/test/data/dom_automation_test_with_popup.html
@@ -0,0 +1,27 @@
+<HTML>
+<HEAD>
+<SCRIPT TYPE="text/javascript">
+<!--
+function popup(mylink, windowname) {
+  if (!window.focus)
+    return true;
+  var href;
+  if (typeof(mylink) == 'string')
+    href=mylink;
+  else
+    href=mylink.href;
+  var popup_window = window.open(href, windowname,
+                                 'width=400,height=200,scrollbars=yes');
+  popup_window.close();
+  return false;
+}
+//-->
+</SCRIPT>
+</HEAD>
+
+<BODY onLoad="return popup(this, 'test popup')">
+<br>Tests the case where an instance of the DOM Automation Controller bound to a
+<br>popup is destroyed. Existing DOM Automation Controller instances should continue
+<br>to work.
+</BODY>
+</HTML>