Fix resource leak on extension window closing.

chrome/browser/chromeos/arc/arc_support_host.cc

 // Copyright 2016 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/chromeos/arc/arc_support_host.h"
 
 #include <string>
 
 #include "ash/common/system/chromeos/devicetype_utils.h"
 #include "base/i18n/timezone.h"
@@ skipping 29 matching lines @@
 const char kOn[] = "on";
 const char kPage[] = "page";
 const char kStatus[] = "status";
 const char kText[] = "text";
 const char kActionInitialize[] = "initialize";
 const char kActionSetMetricsMode[] = "setMetricsMode";
 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
 const char ArcSupportHost::kHostName[] = "com.google.arc_support";
 
 // static
 const char ArcSupportHost::kHostAppId[] = "cnbgggchhmkkdmeppjobngjoejnihlei";
 
 // static
 const char ArcSupportHost::kStorageId[] = "arc_support";
@@ skipping 34 matching lines @@
                  base::Unretained(this)));
 }
 
 ArcSupportHost::~ArcSupportHost() {
   display::Screen::GetScreen()->RemoveObserver(this);
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   if (arc_auth_service)
     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;
   }
 
   SendMetricsMode();
   SendBackupAndRestoreMode();
   SendLocationServicesMode();
 
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   DCHECK(arc_auth_service);
   OnOptInUIShowPage(arc_auth_service->ui_page(),
@@ skipping 149 matching lines @@
 }
 
 void ArcSupportHost::SendBackupAndRestoreMode() {
   SendOptionMode(kActionBackupAndRestoreMode, prefs::kArcBackupRestoreEnabled);
 }
 
 void ArcSupportHost::SendLocationServicesMode() {
   SendOptionMode(kActionLocationServiceMode, prefs::kArcLocationServiceEnabled);
 }
 
 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,
                                        const base::string16& status) {
   if (!client_)
     return;
 
   base::DictionaryValue response;
   response.SetString(kAction, kActionShowPage);
   response.SetInteger(kPage, static_cast<int>(page));
@@ skipping 41 matching lines @@
 
   if (action == kActionStartLso) {
     arc_auth_service->StartLso();
   } else if (action == kActionSetAuthCode) {
     std::string code;
     if (!request->GetString(kCode, &code)) {
       NOTREACHED();
       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)) {
       NOTREACHED();
       return;
     }
     EnableMetrics(is_enabled);
   } else if (action == kActionSendFeedback) {
     chrome::OpenFeedbackDialog(nullptr);
   } else if (action == kActionSetBackupRestore) {
@@ skipping 12 matching lines @@
     EnableLocationService(is_enabled);
   } else {
     NOTREACHED();
   }
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> ArcSupportHost::task_runner()
     const {
   return base::ThreadTaskRunnerHandle::Get();
 }