Fix occasional crash in ~VPNListView during shell destruction.

ash/system/chromeos/network/vpn_list_view.cc

 // Copyright 2015 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 "ash/system/chromeos/network/vpn_list_view.h"
 
 #include <utility>
 #include <vector>
 
 #include "ash/metrics/user_metrics_recorder.h"
@@ skipping 221 matching lines @@
 
 }  // namespace
 
 VPNListView::VPNListView(ui::NetworkListDelegate* delegate)
     : delegate_(delegate) {
   Shell::GetInstance()->system_tray_delegate()->GetVPNDelegate()->AddObserver(
       this);
 }
 
 VPNListView::~VPNListView() {
-  Shell::GetInstance()
-      ->system_tray_delegate()
-      ->GetVPNDelegate()
-      ->RemoveObserver(this);
+  // We need the check as on shell destruction, the delegate is destroyed first.

[bartfab (slow), 2015/10/16 15:45:25]

You are masking a bug here: VPNDelegate expects all observers to unregister
before it is destroyed. If the system trayl delegate and VPNDelegate get
destroyed first, the VPNList will still be registered as an observer, causing a
DCHECK() in the destructor.

The correct fix is for the VPNListView to observe the system tray delegate's
lifetime so that it can unregister itself as on bserver on time - or
alternatively, for the VPNListView to be explicitly destroyed before the system
tray delegate.

[stevenjb, 2015/10/16 16:50:07]

This is actually correct (but see note below).

* VPNListView is a View class owned by the views hierarchy, not by the class
that instantiates it. Because of this it may get destroyed after
SystemTrayDelegate.
* Any delegate returned by system_tray_delegate->GetVPNDelegate() must be owned
by the system tray delegate, so must already be destroyed if
system_tray_delegate is destroyed, so we do not need to explicitly remove this
as an observer.

Furthermore, SystemTrayDelegate is not guaranteed to have a VPNDelegate, so we
should also test that GetVPNDelegate() is not null before calling
RemoveObserver.

[bartfab (slow), 2015/10/16 16:55:22]

VPNDelegate will currently DCHECK() in its destructor if all observers have not
unregistered before the delegate's destruction. We need to either relax this
restriciton (dangerous) or introduce a way for the VPNListView to observe the
delegate's lifetime so that it can unregister itself in time.

[stevenjb, 2015/10/16 17:46:18]

I see. I think this pattern (without the DCHECK on delegate destruction) is
pretty common with Views delegates, but if we want to preserve the DCHECK then
it seems like VPNDelegate should inform its observers that it is shutting down
(rather than observing yet another class here).

I am fine with either solution.

[emaxx, 2015/10/16 17:58:10]

Bartosz, first of all, thanks for pointing at the DCHECK issue.

So your proposed solution is to remove the observer when the delegate is going
to be deleted; but isn't it overcomplicating the things?
This solution will be almost equivalent to just ignoring the left observers (and
disabling the DCHECK), as is currently done in at least two similar places:
DateDefaultView and TraySupervisedUser:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/date/da...
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/chromeo...
So the only benefit of the safer solution, AFAICS, is that if in some future a
new VPNDelegate observer will appear and if it will have the same
destruction-order problem, then this could probably be caught by the DCHECK.
Do you think that's worth the efforts?

[emaxx, 2015/10/16 17:58:10]

I think the idea behind the existing code is that the VPNListView could not have
been constructed without an existing VPNDelegate and that the VPNDelegate should
not be gone for no reason.
Please correct me if that's wrong; in that case we will have to add the checks
in a bunch of places.

[stevenjb, 2015/10/16 18:06:40]

I'm not sure if that is correct. At one point we supported an ash_shell
executable target that has a system tray but uses DefaultSystemTrayDelegate.
There's no reason why it shouldn't be able to have a VPNListView without a
delegate. NetworkStateListDetailedView e.g. does not assume that
GetNetworkingConfigDelegate() doesn't return null.

We should strive to write code against the API and not make assumptions about
the implementation. In this case the API allows the delegate to be null.

[emaxx, 2015/10/28 15:14:38]

Done.

+  SystemTrayDelegate* const system_tray_delegate =
+      Shell::GetInstance()->system_tray_delegate();
+  if (system_tray_delegate)
+    system_tray_delegate->GetVPNDelegate()->RemoveObserver(this);
 }
 
 void VPNListView::Update() {
   // Before updating the list, determine whether the user was hovering over one
   // of the VPN provider or network entries.
   scoped_ptr<VPNProvider::Key> hovered_provider_key;
   std::string hovered_network_service_path;
   for (const std::pair<const views::View* const, VPNProvider::Key>& provider :
        provider_view_key_map_) {
     if (static_cast<const HoverHighlightView*>(provider.first)->hover()) {
@@ skipping 154 matching lines @@
     }
   }
 
   // Add providers without any configured networks, in the order that the
   // providers were returned by the extensions system.
   for (const VPNProvider& provider : providers)
     AddProviderAndNetworks(provider.key, provider.name, networks);
 }
 
 }  // namespace ash