Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410033002:
Fix occasional crash in ~VPNListView during shell destruction. (Closed)
|
Created:
5 years, 2 months ago by emaxx Modified:
5 years, 1 month ago CC:
chromium-reviews, sadrul, stevenjb+watch_chromium.org, oshima+watch_chromium.org, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix occasional crash in ~VPNListView during shell destruction.
The crash is caused by the fact that during the ash::Shell destruction the
SystemTrayDelegate object is destroyed before the child windows are closed -
but apparently the VPNListView destructor accesses SystemTrayDelegate's members
without any checks.
BUG=542146
TEST=manual testing
Committed: https://crrev.com/b0171a54ebf5185259fbb741d771c573d0934ad7
Cr-Commit-Position: refs/heads/master@{#357088}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Disable DCHECK in VPNDelegate; handle possible non-existence of VPNDelegate during VPNListView dest… #
Messages
Total messages: 17 (2 generated)
emaxx@chromium.org changed reviewers: + bartfab@chromium.org, stevenjb@chromium.org
bartfab, stevenjb: Hi, PTAL at this crash-fixing patch.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. 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.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 15:45:25, bartfab (slow) wrote: > 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. 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.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 16:50:07, stevenjb wrote: > On 2015/10/16 15:45:25, bartfab (slow) wrote: > > 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. > > 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. 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.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 16:55:22, bartfab (slow) wrote: > On 2015/10/16 16:50:07, stevenjb wrote: > > On 2015/10/16 15:45:25, bartfab (slow) wrote: > > > 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. > > > > 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. > > 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. 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.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 16:55:22, bartfab (slow) wrote: > On 2015/10/16 16:50:07, stevenjb wrote: > > On 2015/10/16 15:45:25, bartfab (slow) wrote: > > > 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. > > > > 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. > > 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. 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? https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 16:50:07, stevenjb wrote: > Furthermore, SystemTrayDelegate is not guaranteed to have a VPNDelegate, so we > should also test that GetVPNDelegate() is not null before calling > RemoveObserver. 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.
https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 17:58:10, emaxx wrote: > On 2015/10/16 16:50:07, stevenjb wrote: > > Furthermore, SystemTrayDelegate is not guaranteed to have a VPNDelegate, so we > > should also test that GetVPNDelegate() is not null before calling > > RemoveObserver. > > 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. 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.
Fixed the DCHECK issue pointed by Bartosz and addressed Steven's comment. Bartosz, I didn't get your opinion from the discussion: do you insist on implementing the delegate destruction notification? https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... File ash/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on shell destruction, the delegate is destroyed first. On 2015/10/16 18:06:40, stevenjb wrote: > On 2015/10/16 17:58:10, emaxx wrote: > > On 2015/10/16 16:50:07, stevenjb wrote: > > > Furthermore, SystemTrayDelegate is not guaranteed to have a VPNDelegate, so > we > > > should also test that GetVPNDelegate() is not null before calling > > > RemoveObserver. > > > > 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. > > 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. Done.
This lgtm
On 2015/10/28 15:14:38, emaxx wrote: > Fixed the DCHECK issue pointed by Bartosz and addressed Steven's comment. > > Bartosz, I didn't get your opinion from the discussion: do you insist on > implementing the delegate destruction notification? My concern is that there is a VPNDelegate::RemoveObserver() method but we no longer require observers to unregister themselves before VPNDelegate is gone. The danger this creates is that an observer outlives the VPNDelegate, the observer has no idea that the VPNDelegate is already gone and then tries to unregister itself by calling VPNDelegate::RemoveObserver() on already-destroyed VPNDelegate. The only way to avoid this is for the VPNDelegate to inform its observers when it is about to be destroyed and give them a chance to unregister themselves at that moment. > > https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... > File ash/system/chromeos/network/vpn_list_view.cc (right): > > https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... > ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as on > shell destruction, the delegate is destroyed first. > On 2015/10/16 18:06:40, stevenjb wrote: > > On 2015/10/16 17:58:10, emaxx wrote: > > > On 2015/10/16 16:50:07, stevenjb wrote: > > > > Furthermore, SystemTrayDelegate is not guaranteed to have a VPNDelegate, > so > > we > > > > should also test that GetVPNDelegate() is not null before calling > > > > RemoveObserver. > > > > > > 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. > > > > 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. > > Done.
VPNDelegate, like many other UI delegates, is owned by the UI that uses it and is expected to outlive any *active* use of the UI. There are a number of examples of that pattern in the SystemTray UI. The challenge is that the Views hierarchy is destroyed asynchronously which is why this edge-case bug exists. There is no risk here that VPNListView (or any other View) will attempt to access the delegate after it has been destroyed (except in the View destructor). I do understand your concern, and agree that the code would be theoretically more robust with an OnShutdown event in VPNDelegate::Observer, but personally I don't think that the added complexity is necessary or even advantageous here. On Wed, Oct 28, 2015 at 12:05 PM, <bartfab@chromium.org> wrote: > On 2015/10/28 15:14:38, emaxx wrote: > >> Fixed the DCHECK issue pointed by Bartosz and addressed Steven's comment. >> > > Bartosz, I didn't get your opinion from the discussion: do you insist on >> implementing the delegate destruction notification? >> > > My concern is that there is a VPNDelegate::RemoveObserver() method but we > no > longer require observers to unregister themselves before VPNDelegate is > gone. > The danger this creates is that an observer outlives the VPNDelegate, the > observer has no idea that the VPNDelegate is already gone and then tries to > unregister itself by calling VPNDelegate::RemoveObserver() on > already-destroyed > VPNDelegate. The only way to avoid this is for the VPNDelegate to inform > its > observers when it is about to be destroyed and give them a chance to > unregister > themselves at that moment. > > > > > > https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... > >> File ash/system/chromeos/network/vpn_list_view.cc (right): >> > > > > https://codereview.chromium.org/1410033002/diff/1/ash/system/chromeos/network... > >> ash/system/chromeos/network/vpn_list_view.cc:242: // We need the check as >> on >> shell destruction, the delegate is destroyed first. >> On 2015/10/16 18:06:40, stevenjb wrote: >> > On 2015/10/16 17:58:10, emaxx wrote: >> > > On 2015/10/16 16:50:07, stevenjb wrote: >> > > > Furthermore, SystemTrayDelegate is not guaranteed to have a >> VPNDelegate, >> so >> > we >> > > > should also test that GetVPNDelegate() is not null before calling >> > > > RemoveObserver. >> > > >> > > 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. >> > >> > 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. >> > > Done. >> > > https://codereview.chromium.org/1410033002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I still find the pattern of "maybe the observer goes away first, maybe the observed goes away first - no way to know" troubling but I am not an owner of the settings UI. If you are OK with the pattern, LGTM.
The CQ bit was checked by emaxx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410033002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b0171a54ebf5185259fbb741d771c573d0934ad7 Cr-Commit-Position: refs/heads/master@{#357088} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
