Fix resource leak on extension window closing.

chrome/browser/chromeos/arc/arc_support_host.cc

Index: chrome/browser/chromeos/arc/arc_support_host.cc
diff --git a/chrome/browser/chromeos/arc/arc_support_host.cc b/chrome/browser/chromeos/arc/arc_support_host.cc
index fe4452513f15e4e0ae5d20576fa8fab5bfca5fae..9fd69720979e3c56b70f03ed264230704d1025eb 100644
--- a/chrome/browser/chromeos/arc/arc_support_host.cc
+++ b/chrome/browser/chromeos/arc/arc_support_host.cc
@@ -47,14 +47,17 @@ const char kActionBackupAndRestoreMode[] = "setBackupAndRestoreMode";
 const char kActionLocationServiceMode[] = "setLocationServiceMode";
 const char kActionSetWindowBounds[] = "setWindowBounds";
 const char kActionStartLso[] = "startLso";
-const char kActionCancelAuthCode[] = "cancelAuthCode";
 const char kActionSetAuthCode[] = "setAuthCode";
 const char kActionEnableMetrics[] = "enableMetrics";
 const char kActionSendFeedback[] = "sendFeedback";
 const char kActionSetBackupRestore[] = "setBackupRestore";
 const char kActionSetLocationService[] = "setLocationService";
-const char kActionCloseUI[] = "closeUI";
+const char kActionCloseWindow[] = "closeWindow";
 const char kActionShowPage[] = "showPage";
+
+// Fired when the extension window is closed.
+const char kActionOnWindowClosed[] = "onWindowClosed";
+
 }  // namespace
 
 // static
@@ -109,12 +112,24 @@ ArcSupportHost::~ArcSupportHost() {
     arc_auth_service->RemoveObserver(this);
 }
 
+void ArcSupportHost::Close() {
+  if (!client_)
+    return;
+
+  close_requested_ = true;
+  base::DictionaryValue response;
+  response.SetString(kAction, kActionCloseWindow);
+  std::string response_string;
+  base::JSONWriter::Write(response, &response_string);
+  client_->PostMessageFromNativeHost(response_string);
+}
+
 void ArcSupportHost::Start(Client* client) {
   DCHECK(!client_);
   client_ = client;
 
   if (!Initialize()) {
-    OnOptInUIClose();
+    Close();
     return;
   }
 
@@ -285,14 +300,7 @@ void ArcSupportHost::SendLocationServicesMode() {
 }
 
 void ArcSupportHost::OnOptInUIClose() {

[Luis Héctor Chávez, 2016/09/30 15:14:14]

Given that this and Close() are now equivalent, can we get rid of one? Do we
need Close()? Can it not be OnOptInUIClose()?

[hidehiko, 2016/09/30 15:21:25]

I'm planning to remove OnOptInUIClose() as well as OnOptInUIShowPage(), because
these are actually not a observer, but delegate-like code. OnOptInUIClose() is
not an event that the window is closed, but just a request to close the window
from ArcAuthService to ArcSupportHost, which does not sound Observer and is
confusing.

Unfortunately, there is circular dependency. Cleaning the interface needs this
fix, otherwise multiple ArcSupportHost instances conflict to access
ArcAuthService. On the other hand, to simplify this CL, the dependency fix is
needed, as you pointed.

So, I'd like to fix the resource-leak issue first, and then fix the code
dependency.

-  if (!client_)
-    return;
-
-  base::DictionaryValue response;
-  response.SetString(kAction, kActionCloseUI);
-  std::string response_string;
-  base::JSONWriter::Write(response, &response_string);
-  client_->PostMessageFromNativeHost(response_string);
+  Close();
 }
 
 void ArcSupportHost::OnOptInUIShowPage(arc::ArcAuthService::UIPage page,
@@ -354,8 +362,9 @@ void ArcSupportHost::OnMessage(const std::string& request_string) {
       return;
     }
     arc_auth_service->SetAuthCodeAndStartArc(code);
-  } else if (action == kActionCancelAuthCode) {
-    arc_auth_service->CancelAuthCode();
+  } else if (action == kActionOnWindowClosed) {
+    if (!close_requested_)
+      arc_auth_service->CancelAuthCode();
   } else if (action == kActionEnableMetrics) {
     bool is_enabled;
     if (!request->GetBoolean(kEnabled, &is_enabled)) {