Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1464433002:
Check that child window was not destroyed inside UpdateTransientChildVisibility. (Closed)
DescriptionCheck that child window was not destroyed inside UpdateTransientChildVisibility.
Now it is possible to remove window inside the loop in
TransientWindowManager::OnWindowVisibilityChanged. So
ransient_children_ would contain dangling pointers.
BUG=
Committed: https://crrev.com/62c0b6b8271eb1cc669dd6cbb6a6d4782863a48a
Cr-Commit-Position: refs/heads/master@{#361285}
Patch Set 1 #
Total comments: 2
Patch Set 2 : nits #
Total comments: 5
Patch Set 3 : Using WindowsTracker #
Total comments: 1
Patch Set 4 : Added comment #
Messages
Total messages: 26 (8 generated)
Description was changed from ========== Check that child window was not destroyed inside UpdateTransientChildVisibility. Now it is possible to remove window inside the loop in TransientWindowManager::OnWindowVisibilityChanged. So ransient_children_ would contain dangling pointers. BUG= ========== to ========== Check that child window was not destroyed inside UpdateTransientChildVisibility. Now it is possible to remove window inside the loop in TransientWindowManager::OnWindowVisibilityChanged. So ransient_children_ would contain dangling pointers. BUG= ==========
kirr@yandex-team.ru changed reviewers: + oshima@chromium.org, sky@chromium.org
I'm not sure if such crash exists in Chromium UI. But it's possible in common.
lgtm with nits https://codereview.chromium.org/1464433002/diff/1/ui/wm/core/transient_window... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/1/ui/wm/core/transient_window... ui/wm/core/transient_window_manager.cc:169: child) != transient_children_.end()) you need {} in this case https://codereview.chromium.org/1464433002/diff/1/ui/wm/core/transient_window... File ui/wm/core/transient_window_manager_unittest.cc (right): https://codereview.chromium.org/1464433002/diff/1/ui/wm/core/transient_window... ui/wm/core/transient_window_manager_unittest.cc:65: }; nit: DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by kirr@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1464433002/#ps20001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464433002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464433002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kirr@yandex-team.ru changed reviewers: + sadrul@chromium.org
On 2015/11/20 09:10:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Needed LGTM from a files owner.
https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: // Child window can be removed inside UpdateTransientChildVisibility call. Your description says some of the children can be destroyed. If that's the case, I don't like checking for pointer values to determine validity.
https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: // Child window can be removed inside UpdateTransientChildVisibility call. On 2015/11/20 15:56:57, sky wrote: > Your description says some of the children can be destroyed. If that's the case, > I don't like checking for pointer values to determine validity. Sorry if I'm missing something, but the window lifetime is observed explicitly by OnWindowDestroying, so it looks to me that it's checking if it has been destroyed (like WindowTracker)?
https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: // Child window can be removed inside UpdateTransientChildVisibility call. On 2015/11/20 18:39:18, oshima wrote: > On 2015/11/20 15:56:57, sky wrote: > > Your description says some of the children can be destroyed. If that's the > case, > > I don't like checking for pointer values to determine validity. > > Sorry if I'm missing something, but the window lifetime is observed explicitly > by OnWindowDestroying, so it looks to me that it's checking if it has been > destroyed (like WindowTracker)? The copy of the windows (transient_children) is referencing a deleted window. The change assumes a window with the same value hasn't been created.
https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: // Child window can be removed inside UpdateTransientChildVisibility call. On 2015/11/20 18:53:44, sky wrote: > On 2015/11/20 18:39:18, oshima wrote: > > On 2015/11/20 15:56:57, sky wrote: > > > Your description says some of the children can be destroyed. If that's the > > case, > > > I don't like checking for pointer values to determine validity. > > > > Sorry if I'm missing something, but the window lifetime is observed explicitly > > by OnWindowDestroying, so it looks to me that it's checking if it has been > > destroyed (like WindowTracker)? > > The copy of the windows (transient_children) is referencing a deleted window. > The change assumes a window with the same value hasn't been created. If a new window is created at the same address, it won't be in the transient_children_, unless it's explicitly added as a transient children, in which case, the visibility will be updated with this CL, but will not with WindowTracker. I don't have strong opinion about which behavior is better. Such code (adding transient children during visibility change) should probably be avoided anyway.
On 2015/11/20 19:07:26, oshima wrote: > https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... > File ui/wm/core/transient_window_manager.cc (right): > > https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... > ui/wm/core/transient_window_manager.cc:167: // Child window can be removed > inside UpdateTransientChildVisibility call. > On 2015/11/20 18:53:44, sky wrote: > > On 2015/11/20 18:39:18, oshima wrote: > > > On 2015/11/20 15:56:57, sky wrote: > > > > Your description says some of the children can be destroyed. If that's the > > > case, > > > > I don't like checking for pointer values to determine validity. > > > > > > Sorry if I'm missing something, but the window lifetime is observed > explicitly > > > by OnWindowDestroying, so it looks to me that it's checking if it has been > > > destroyed (like WindowTracker)? > > > > The copy of the windows (transient_children) is referencing a deleted window. > > The change assumes a window with the same value hasn't been created. > > If a new window is created at the same address, it won't be in the > transient_children_, unless it's explicitly added as a transient children, in > which case, the visibility will be updated with this CL, but will not with > WindowTracker. > > I don't have strong opinion about which behavior is better. Such code (adding > transient children during visibility change) should probably be avoided anyway. Seems like the right way to fix it is complicated. We need to use some ObserverList - like container, instead std::vector and needed to do something with transient_children() method in interface (maybe copy this container every time or use one more). I'm not sure are such changes needed.
https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:171: } Maybe this could use a WindowTracker? e.g. WindowTracker tracker; for (Window* child : transient_children_) tracker.Add(child); while (!tracker.windows().empty()) { Window* window = *(tracker.windows().begin()); Get(window)->Update...(); tracker.Remove(window); }
On 2015/11/21 08:42:44, sadrul wrote: > https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... > File ui/wm/core/transient_window_manager.cc (right): > > https://codereview.chromium.org/1464433002/diff/20001/ui/wm/core/transient_wi... > ui/wm/core/transient_window_manager.cc:171: } > Maybe this could use a WindowTracker? e.g. > > WindowTracker tracker; > for (Window* child : transient_children_) > tracker.Add(child); > > while (!tracker.windows().empty()) { > Window* window = *(tracker.windows().begin()); > Get(window)->Update...(); > tracker.Remove(window); > } Thanks a lot. Did this way,
https://codereview.chromium.org/1464433002/diff/40001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/1464433002/diff/40001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:166: aura::WindowTracker tracker; Add a comment as to why you use WindowTracker.
On 2015/11/22 15:24:16, sky wrote: > Add a comment as to why you use WindowTracker. Done.
LGTM
The CQ bit was checked by kirr@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1464433002/#ps60001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464433002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464433002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/62c0b6b8271eb1cc669dd6cbb6a6d4782863a48a Cr-Commit-Position: refs/heads/master@{#361285} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
