arc: Fix situation when ARC cannot be disabled safely.

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 <utility>
 
 #include "ash/common/system/chromeos/devicetype_utils.h"
@@ skipping 347 matching lines @@
     Close();
   } else if (ui_page_ == UIPage::ERROR) {
     ShowError(error_, should_show_send_feedback_);
   } else {
     ShowPage(ui_page_);
   }
 }
 
 void ArcSupportHost::UnsetMessageHost(
     arc::ArcSupportMessageHost* message_host) {
-  if (message_host_ != message_host)
-    return;
-  DisconnectMessageHost();
+  if (message_host_ == message_host)
+    DisconnectMessageHost();
+
+  // There is possible scenario when page is requested to close and then
+  // requested to show again soon. In this case OpenApplication may be ignored

[hidehiko, 2017/01/05 08:18:02]

Who ignores the OpenApplication()?
I couldn't find such handling code in, e.g., application_launch.cc etc.?
Could you point me the condition?

We used to hit a bug that multiple extension processes were launched
crbug.com/651838. So, at least at the time, OpenApplication could launch
multiple processes. Actually, this class's design relies on it. At the moment, I
couldn't find design change. Do you know something?
If there is, we need to re-design the whole mechanism.
Or, if not, maybe there is some more specific condition where the window is not
opened, so the specific one needs to be addressed, instead. WDYT?

# Considering that, I skipped other comments about this block. I'll revisit
here, when above things are resolved.

[khmel, 2017/01/05 22:55:53]

Sorry, I don't exactly know code that prevents running multiple instanceso of
this UI and currently have no time for such investigations. I have suspicion
this due unique window id "play_store_wnd" in system. To be double sure I wrote
test that run periodically RequestOpenApp. In my case I have only one window
open and only one SetMessageHost regardless of how many times it was requested
to open. Previous impl was built relying on this feature. If current design
relies that multiple instances of platform app can be running then this might
mean that design does reflect what actually happens in system and we might want
revise it.
Anyway, if you strictly against this fix I can drop this change because
arc_session_manager fix is more important and with that fix this problem is much
less visible.

[hidehiko, 2017/01/06 14:25:36]

I'm not against fixing ArcSupportHost, rather if there is a bug, we should fix
it.
However, in general, if the code author does not know the code and its
background logic in details, we cannot say l-g-t-m. Specifically, this case has
some contradiction with a past bug, so it looks more important.

Note that, if "play_store_wnd" id is the root cause as you suspected (sorry I do
not have enough time to investigate it today), it explains both the bug you're
trying to fix and the past bug at a time. However, before moving forward, it is
necessary to verify it rather than suspecting (Unfortunately, there are more
layers, so running RequestOpenApp multiple times looks not support it
enough...). Also, then, unfortunately, this code would introduces very
complicated message passing flows which make maintenance very difficult, and
actually would have a race issue, so it would be necessary to think about
another way.

Anyway, arc_session_manager only fix sounds more simpler, and easily
understandable. Thank you for helping review.

[khmel, 2017/01/10 02:17:12]

Acknowledged.

+  // because app is still
+  // active. Now, when app is finally closed and pending request is open,
+  // try to open app again.
+  if (app_start_pending_)
+    request_open_app_callback_.Run(profile_);
 }
 
 void ArcSupportHost::DisconnectMessageHost() {
   DCHECK(message_host_);
   display::Screen* screen = display::Screen::GetScreen();
   if (screen)
     screen->RemoveObserver(this);
   message_host_->SetObserver(nullptr);
   message_host_ = nullptr;
 }
@@ skipping 163 matching lines @@
     for (auto& observer : observer_list_)
       observer.OnRetryClicked();
   } else if (event == kEventOnSendFeedbackClicked) {
     for (auto& observer : observer_list_)
       observer.OnSendFeedbackClicked();
   } else {
     LOG(ERROR) << "Unknown message: " << event;
     NOTREACHED();
   }
 }