Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047083002:
Do not allow NULL to be passed into FocusManager::ViewRemoved. (Closed)
DescriptionDo not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL.
This could happen when a top view is focused and it's being destroyed.
BUG=617305
Committed: https://crrev.com/3f785a8c6d9e355eef6ea0b09c6df1eaedc850f4
Cr-Commit-Position: refs/heads/master@{#400174}
Patch Set 1 #Patch Set 2 : Ignore cases when focused_view_ == NULL #Patch Set 3 : may be fix #Patch Set 4 : reduce diff #
Total comments: 8
Patch Set 5 : simplify DCHECK, add a comment. #Patch Set 6 : do not allow NULL at all #Messages
Total messages: 46 (17 generated)
The CQ bit was checked by krasin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by krasin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by krasin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krasin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/60001
Description was changed from ========== Do not allow NULL to be passed into FocusManager::ViewRemoved. BUG=617305 ========== to ========== Do not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL. This could happen when a top view is focused and it's being destroyed. BUG=617305 ==========
krasin@google.com changed reviewers: + sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Scott, friendly ping.
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) Contains handles null correctly, so you shouldn't need this early out. https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:522: DCHECK(removed != NULL); DCHECK(removed);
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) On 2016/06/09 17:23:14, sky wrote: > Contains handles null correctly, so you shouldn't need this early out. I need an early exit, if ViewRemoved is called with focused_view_==NULL and removed==NULL, which is happening all the time: https://crbug.com/617305#c6 https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:522: DCHECK(removed != NULL); On 2016/06/09 17:23:14, sky wrote: > DCHECK(removed); Done.
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) On 2016/06/09 17:46:11, krasin wrote: > On 2016/06/09 17:23:14, sky wrote: > > Contains handles null correctly, so you shouldn't need this early out. > > I need an early exit, if ViewRemoved is called with focused_view_==NULL and > removed==NULL, which is happening all the time: https://crbug.com/617305#c6 Not sure I follow. If focused_view_ is null Contains(null) returns false. In other words, I don't think you need an early out. But maybe I'm missing something?
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) Consider the case, when removed == NULL. We have two cases: 1. Common, when focused_view_ == NULL as well; 2. Uncommon, fixed in this CL: focused_view != NULL. It means that I can't put the DCHECK(removed) into the beginning of the function, as it will fire on the case 1 as well. That's why we need the condition prior to the DCHECK with an early exit.
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) On 2016/06/09 18:13:06, krasin wrote: > Consider the case, when removed == NULL. removed == null is an error in the call sites and should be fixed. That's why you're fixing ui/views/widget/widget.cc. In case I'm not being clear this code should look like: void FocusManager::ViewRemoved(View* removed) { DCHECK(removed); // We should never be supplied a null view. if (removed->Contains(focused_view_)) SetFocusedView(nullptr); } > We have two cases: > > 1. Common, when focused_view_ == NULL as well; > 2. Uncommon, fixed in this CL: focused_view != NULL. > > It means that I can't put the DCHECK(removed) into the beginning of the > function, as it will fire on the case 1 as well. > > That's why we need the condition prior to the DCHECK with an early exit.
https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:520: if (!focused_view_) On 2016/06/09 22:34:24, sky wrote: > On 2016/06/09 18:13:06, krasin wrote: > > Consider the case, when removed == NULL. > > removed == null is an error in the call sites and should be fixed. That's why > you're fixing ui/views/widget/widget.cc. > > In case I'm not being clear this code should look like: > > void FocusManager::ViewRemoved(View* removed) { > DCHECK(removed); // We should never be supplied a null view. > if (removed->Contains(focused_view_)) > SetFocusedView(nullptr); > } > > > > We have two cases: > > > > 1. Common, when focused_view_ == NULL as well; > > 2. Uncommon, fixed in this CL: focused_view != NULL. > > > > It means that I can't put the DCHECK(removed) into the beginning of the > > function, as it will fire on the case 1 as well. > > > > That's why we need the condition prior to the DCHECK with an early exit. > I have tried that. Multiple (almost all) tests are broken in this case, see the very first diff in the current CL: https://codereview.chromium.org/2047083002/#ps1 If you believe it's always wrong to pass NULL into this function, I will try to fix all the callers.
I looked at a handful of the failures and they are originated from the place you're fixing. Very few places directly call ViewRemoved. -Scott On Thu, Jun 9, 2016 at 4:10 PM, <krasin@google.com> wrote: > > https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... > File ui/views/focus/focus_manager.cc (right): > > https://codereview.chromium.org/2047083002/diff/60001/ui/views/focus/focus_ma... > ui/views/focus/focus_manager.cc:520: if (!focused_view_) > On 2016/06/09 22:34:24, sky wrote: >> On 2016/06/09 18:13:06, krasin wrote: >> > Consider the case, when removed == NULL. >> >> removed == null is an error in the call sites and should be fixed. > That's why >> you're fixing ui/views/widget/widget.cc. >> >> In case I'm not being clear this code should look like: >> >> void FocusManager::ViewRemoved(View* removed) { >> DCHECK(removed); // We should never be supplied a null view. >> if (removed->Contains(focused_view_)) >> SetFocusedView(nullptr); >> } >> >> >> > We have two cases: >> > >> > 1. Common, when focused_view_ == NULL as well; >> > 2. Uncommon, fixed in this CL: focused_view != NULL. >> > >> > It means that I can't put the DCHECK(removed) into the beginning of > the >> > function, as it will fire on the case 1 as well. >> > >> > That's why we need the condition prior to the DCHECK with an early > exit. >> > > I have tried that. Multiple (almost all) tests are broken in this case, > see the very first diff in the current CL: > https://codereview.chromium.org/2047083002/#ps1 > > If you believe it's always wrong to pass NULL into this function, I will > try to fix all the callers. > > https://codereview.chromium.org/2047083002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by krasin@google.com to run a CQ dry run
Okay, I will try. Sending a dry CQ run to see what would fail.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/100001
Whoa! Actually, that looks pretty good. Just three more trybots to go.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by krasin@google.com
The CQ bit was unchecked by krasin@google.com
Scott, does the fix look good in its current form? If yes, I would probably submit it as is.
Friendly ping. All trybots passed.
Ping! Today is the last day before I go on vacation. While technically I could continue working on this there, it would be defeating the point. Please, review the CL. All try bots passed and all your comments addressed.
LGTM - sorry, was OOO until today.
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047083002/100001
Message was sent while issue was closed.
Description was changed from ========== Do not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL. This could happen when a top view is focused and it's being destroyed. BUG=617305 ========== to ========== Do not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL. This could happen when a top view is focused and it's being destroyed. BUG=617305 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Do not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL. This could happen when a top view is focused and it's being destroyed. BUG=617305 ========== to ========== Do not allow NULL to be passed into FocusManager::ViewRemoved, when focused_view_ != NULL. This could happen when a top view is focused and it's being destroyed. BUG=617305 Committed: https://crrev.com/3f785a8c6d9e355eef6ea0b09c6df1eaedc850f4 Cr-Commit-Position: refs/heads/master@{#400174} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3f785a8c6d9e355eef6ea0b09c6df1eaedc850f4 Cr-Commit-Position: refs/heads/master@{#400174} |
