Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1686433002:
Remove DialogDelegate::OnClosed() which is redundant with (Closed)
|
Created:
4 years, 10 months ago by Evan Stade Modified:
4 years, 10 months ago CC:
chromium-reviews, tim+watch_chromium.org, sadrul, vabr+watchlistpasswordmanager_chromium.org, zea+watch_chromium.org, tfarina, maxbogue+watch_chromium.org, plaree+watch_chromium.org, oshima+watch_chromium.org, kalyank, gcasto+watchlist_chromium.org, davemoore+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove DialogDelegate::OnClosed() which is redundant with
WidgetDelegate::WindowClosing().
BUG=585312, 583330
TBR=avi@chromium.org
Committed: https://crrev.com/6f69f42209045c9d0c8b6bfc44d83ca6eca70a74
Cr-Commit-Position: refs/heads/master@{#377057}
Patch Set 1 #Patch Set 2 : fix a browsertest #Patch Set 3 : fix another test #Patch Set 4 : rebase #
Total comments: 17
Patch Set 5 : review #
Total comments: 1
Patch Set 6 : . #Patch Set 7 : change less #
Total comments: 4
Patch Set 8 : msw nits #Patch Set 9 : rebase #Messages
Total messages: 69 (29 generated)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Description was changed from ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=582675 ========== to ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312 ==========
The CQ bit was unchecked by estade@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
estade@chromium.org changed reviewers: + msw@chromium.org, vasilii@chromium.org
+msw could you review the overall change? +vasilii could you review the changes to passwords stuff?
https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:107: // This method is called twice. crbug.com/583330 Assuming that the method isn't called twice you can remove the line #108 and close the bug. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (left): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:295: EXPECT_CALL(*controller(), OnDialogClosed()); It should be called when the autosignin dialog is destroyed. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:436: EXPECT_CALL(*controller(), OnDialogClosed()); Same as above https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:385: content::RunAllPendingInMessageLoop(); Is it now async? You can drop http://crbug.com/583330 reference if you close it with this CL. https://codereview.chromium.org/1686433002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/1686433002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:131: notified_delegate_ = true; I suspect that you can drop |notified_delegate_|
https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:166: modal_dialog->Show(); I suppose it's potentially necessary to create the Widget, in order to keep the changed below working, but is it necessary to actually show the dialog? Otherwise, I wonder if showing a dialog here might actually interfere with other tests running simultaneously (but any tests that depend on focus/activation should be interative_ui_tests). https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:182: delegate_view->GetDialogClientView()->AcceptWindow(); Can you stick with DialogDelegateView::Accept/Cancel + DeleteDelegate in these three cases, and avoid creating the Widget? https://codereview.chromium.org/1686433002/diff/60001/components/app_modal/vi... File components/app_modal/views/javascript_app_modal_dialog_views.cc (right): https://codereview.chromium.org/1686433002/diff/60001/components/app_modal/vi... components/app_modal/views/javascript_app_modal_dialog_views.cc:114: views::Widget* JavaScriptAppModalDialogViews::GetWidget() { nit: reorder these GetWidget impls too, please.
estade@chromium.org changed reviewers: + sky@chromium.org
+sky for dialog_client_view.cc https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:166: modal_dialog->Show(); On 2016/02/11 18:08:58, msw wrote: > I suppose it's potentially necessary to create the Widget, in order to keep the > changed below working, but is it necessary to actually show the dialog? > Otherwise, I wonder if showing a dialog here might actually interfere with other > tests running simultaneously (but any tests that depend on focus/activation > should be interative_ui_tests). i don't think it would interfere with other tests, it's only window-modal https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:182: delegate_view->GetDialogClientView()->AcceptWindow(); On 2016/02/11 18:08:58, msw wrote: > Can you stick with DialogDelegateView::Accept/Cancel + DeleteDelegate in these > three cases, and avoid creating the Widget? that seems like worse coverage. The test seems like it's trying to simulate user actions, and this code is closer to doing that, instead of re-implementing what happens after the user takes an action. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:107: // This method is called twice. crbug.com/583330 On 2016/02/11 10:37:48, vasilii wrote: > Assuming that the method isn't called twice you can remove the line #108 and > close the bug. Done. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (left): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:295: EXPECT_CALL(*controller(), OnDialogClosed()); On 2016/02/11 10:37:48, vasilii wrote: > It should be called when the autosignin dialog is destroyed. well, this is kinda janky. This wasn't being called on destruction which I think was an oversight, although it doesn't even seem very important to call it in this case. The fix is to call it when the controller is destroyed before the dialog is closed. I had to make the change in the prod class as well as the test class because virtual fns don't work in destructors. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:436: EXPECT_CALL(*controller(), OnDialogClosed()); On 2016/02/11 10:37:48, vasilii wrote: > Same as above Done. https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:385: content::RunAllPendingInMessageLoop(); On 2016/02/11 10:37:48, vasilii wrote: > Is it now async? > You can drop http://crbug.com/583330 reference if you close it with this CL. Done. https://codereview.chromium.org/1686433002/diff/60001/components/app_modal/vi... File components/app_modal/views/javascript_app_modal_dialog_views.cc (right): https://codereview.chromium.org/1686433002/diff/60001/components/app_modal/vi... components/app_modal/views/javascript_app_modal_dialog_views.cc:114: views::Widget* JavaScriptAppModalDialogViews::GetWidget() { On 2016/02/11 18:08:58, msw wrote: > nit: reorder these GetWidget impls too, please. Done. https://codereview.chromium.org/1686433002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/1686433002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:131: notified_delegate_ = true; On 2016/02/11 10:37:48, vasilii wrote: > I suspect that you can drop |notified_delegate_| done. I can't repro the original bug on linux because holding space immediately closes the js dialog. Is this a windows-specific thing? Would be nice to verify this doesn't regress that case.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1686433002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.h (left): https://codereview.chromium.org/1686433002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.h:113: bool notified_delegate_; Why is this no longer necessary?
https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (left): https://codereview.chromium.org/1686433002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:295: EXPECT_CALL(*controller(), OnDialogClosed()); On 2016/02/11 20:02:11, Evan Stade wrote: > On 2016/02/11 10:37:48, vasilii wrote: > > It should be called when the autosignin dialog is destroyed. > > well, this is kinda janky. This wasn't being called on destruction which I think > was an oversight, although it doesn't even seem very important to call it in > this case. The fix is to call it when the controller is destroyed before the > dialog is closed. I had to make the change in the prod class as well as the test > class because virtual fns don't work in destructors. Please don't change the production code. The method was called before your CL as the test passed. I wonder why it's not called on dialog destruction anymore.
On 2016/02/12 12:57:09, vasilii wrote: > Please don't change the production code. > The method was called before your CL as the test passed. I wonder why it's not > called on dialog destruction anymore. Is there a particular reason this change to production is bad? I am already changing production code with most of this CL. Changes to production necessitate other changes to production. The dialog is still being destroyed without this change, it's just that the destruction order is changed to after the controller rather than before. We're trying to test whether OnDialogHidden has been called /by the time the controller is destroyed/ and that no longer works unless we make this change to the dtor. But the real thing we care about (it seems to me) is that the delegate is informed that the dialog is being/will be hidden, exactly once, and this change makes (keeps) that true. > Scott said: > Why is this no longer necessary? The theory is that since the delegate notification no longer exists in the same form, this workaround is no longer necessary. But I'll have to do some more investigation about whether that's really true when I'm back in the office Monday.
On 2016/02/12 18:41:15, Evan Stade (ooo) wrote: > On 2016/02/12 12:57:09, vasilii wrote: > > Please don't change the production code. > > The method was called before your CL as the test passed. I wonder why it's not > > called on dialog destruction anymore. > > Is there a particular reason this change to production is bad? I am already > changing production code with most of this CL. Changes to production necessitate > other changes to production. Yes. OnDialogHidden() does a lot of stuff like updating the icon for the closed tab and calling the callback for the web site. The object responsible for handling the callback is tied to WebContents like ManagePasswordsUIController. Thus, there is a potential for use after free. The destructor should be empty. > The dialog is still being destroyed without this change, it's just that the > destruction order is changed to after the controller rather than before. We're > trying to test whether OnDialogHidden has been called /by the time the > controller is destroyed/ and that no longer works unless we make this change to > the dtor. But the real thing we care about (it seems to me) is that the delegate > is informed that the dialog is being/will be hidden, exactly once, and this > change makes (keeps) that true. We test this expectation in PasswordDialogViewTest.PopupAutoSigninPrompt. For the case when everything is destroyed by the framework it doesn't matter.
On 2016/02/15 10:36:46, vasilii wrote: > On 2016/02/12 18:41:15, Evan Stade (ooo) wrote: > > On 2016/02/12 12:57:09, vasilii wrote: > > > Please don't change the production code. > > > The method was called before your CL as the test passed. I wonder why it's > not > > > called on dialog destruction anymore. > > > > Is there a particular reason this change to production is bad? I am already > > changing production code with most of this CL. Changes to production > necessitate > > other changes to production. > > Yes. OnDialogHidden() does a lot of stuff like updating the icon for the closed > tab and calling the callback for the web site. The object responsible for > handling the callback is tied to WebContents like ManagePasswordsUIController. > Thus, there is a potential for use after free. > The destructor should be empty. > > > The dialog is still being destroyed without this change, it's just that the > > destruction order is changed to after the controller rather than before. We're > > trying to test whether OnDialogHidden has been called /by the time the > > controller is destroyed/ and that no longer works unless we make this change > to > > the dtor. But the real thing we care about (it seems to me) is that the > delegate > > is informed that the dialog is being/will be hidden, exactly once, and this > > change makes (keeps) that true. > > We test this expectation in PasswordDialogViewTest.PopupAutoSigninPrompt. For > the case when everything is destroyed by the framework it doesn't matter. not sure I follow. For the case where everything is destroyed by tab contents teardown, you objected when I removed EXPECT_CALL(*controller(), OnDialogClosed());. It sounds like now you're saying it doesn't matter?
On 2016/02/16 18:33:30, Evan Stade (ooo) wrote: > On 2016/02/15 10:36:46, vasilii wrote: > > On 2016/02/12 18:41:15, Evan Stade (ooo) wrote: > > > On 2016/02/12 12:57:09, vasilii wrote: > > > > Please don't change the production code. > > > > The method was called before your CL as the test passed. I wonder why it's > > not > > > > called on dialog destruction anymore. > > > > > > Is there a particular reason this change to production is bad? I am already > > > changing production code with most of this CL. Changes to production > > necessitate > > > other changes to production. > > > > Yes. OnDialogHidden() does a lot of stuff like updating the icon for the > closed > > tab and calling the callback for the web site. The object responsible for > > handling the callback is tied to WebContents like ManagePasswordsUIController. > > Thus, there is a potential for use after free. > > The destructor should be empty. > > > > > The dialog is still being destroyed without this change, it's just that the > > > destruction order is changed to after the controller rather than before. > We're > > > trying to test whether OnDialogHidden has been called /by the time the > > > controller is destroyed/ and that no longer works unless we make this change > > to > > > the dtor. But the real thing we care about (it seems to me) is that the > > delegate > > > is informed that the dialog is being/will be hidden, exactly once, and this > > > change makes (keeps) that true. > > > > We test this expectation in PasswordDialogViewTest.PopupAutoSigninPrompt. For > > the case when everything is destroyed by the framework it doesn't matter. > > not sure I follow. For the case where everything is destroyed by tab contents > teardown, you objected when I removed EXPECT_CALL(*controller(), > OnDialogClosed());. It sounds like now you're saying it doesn't matter? Sorry, it was rather a question. I didn't understand why the expectation has gone. Now I understand that it's due to a different destruction order. I'm OK with what you implemented originally (= dropping EXPECT_CALL).
sky: turns out notified_delegate_ is required after all, even though there's no DialogDelegate::OnClosing any more. |notified_delegate_| didn't actually refer to this function, so to make that clearer I've changed the name of the var to |delegate_allowed_close_|. Also, I don't think we need to check it in AcceptWindow or CancelWindow (perhaps should DCHECK it's not true?), but I left those checks in place regardless. vasilii: restored PS4 wrt passwords dialog and test
lgtm chrome/browser/ui/views/passwords/
LGTM
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping msw, try failures seem unrelated
On 2016/02/18 22:33:39, Evan Stade wrote: > ping msw, try failures seem unrelated ping msw
lgtm with nits https://codereview.chromium.org/1686433002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (left): https://codereview.chromium.org/1686433002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:113: // This method is called twice. crbug.com/583330 nit: note this in BUG= and mark the issue fixed? https://codereview.chromium.org/1686433002/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1686433002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:126: delegate_allowed_close_ = nit: |=
Description was changed from ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312 ========== to ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 ==========
https://codereview.chromium.org/1686433002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (left): https://codereview.chromium.org/1686433002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:113: // This method is called twice. crbug.com/583330 On 2016/02/22 18:44:04, msw wrote: > nit: note this in BUG= and mark the issue fixed? Done. https://codereview.chromium.org/1686433002/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1686433002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:126: delegate_allowed_close_ = On 2016/02/22 18:44:04, msw wrote: > nit: |= I don't think that has the same behavior. We specifically want to avoid calling GetDialogDelegate()->Close() if |delegate_allowed_close_| is already true. Since perhaps that is subtle, I changed this code slightly to make it more obvious.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/140001
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 estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/1686433002/#ps140001 (title: "msw nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/140001
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...)
Description was changed from ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 ========== to ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 TBR=avi@chromium.org ==========
estade@chromium.org changed reviewers: + avi@chromium.org
Going to TBR avi for javascript_app_modal_dialog_views.cc because it originally lived in chrome/browser/ui/views and it could probably be considered an oversight that sky is not an owner of it in the new location.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/140001
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...)
On 2016/02/23 01:23:47, 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...) LGTM And sky should be an owner. Someone with Views experience should be an owner.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, avi@chromium.org, sky@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/1686433002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686433002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686433002/160001
Message was sent while issue was closed.
Description was changed from ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 TBR=avi@chromium.org ========== to ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 TBR=avi@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 TBR=avi@chromium.org ========== to ========== Remove DialogDelegate::OnClosed() which is redundant with WidgetDelegate::WindowClosing(). BUG=585312, 583330 TBR=avi@chromium.org Committed: https://crrev.com/6f69f42209045c9d0c8b6bfc44d83ca6eca70a74 Cr-Commit-Position: refs/heads/master@{#377057} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6f69f42209045c9d0c8b6bfc44d83ca6eca70a74 Cr-Commit-Position: refs/heads/master@{#377057}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1730893002/ by shrike@chromium.org. The reason for reverting is: Failure Builder: Linux Chromium OS ASan LSan Tests (1) https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... ==12959==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000047640 at pc 0x0000008c9c62 bp 0x7ffc51d4ad70 sp 0x7ffc51d4a530 WRITE of size 16 at 0x60e000047640 thread T0 (browser_tests) #0 0x8c9c61 in __asan_memset (/tmp/runKDXLlt/out/Release/browser_tests+0x8c9c61) #1 0x7cb68dc in ash::LogoutConfirmationController::OnDialogClosed() ash/system/chromeos/session/logout_confirmation_controller.cc:83:11 #2 0x5ce779b in views::Widget::OnNativeWidgetDestroying() ui/views/widget/widget.cc:1088:3 #3 0x5d0a61e in OnWindowDestroying ui/views/widget/native_widget_aura.cc:830:3 #4 0x5d0a61e in non-virtual thunk to views::NativeWidgetAura::OnWindowDestroying(aura::Window*) ui/views/widget/native_widget_aura.cc:828 #5 0xde3a857 in aura::Window::~Window() ui/aura/window.cc:109:5 #6 0xde3c02d in aura::Window::~Window() ui/aura/window.cc:102:19 #7 0x7c41d9c in ash::RootWindowController::CloseChildWindows() ash/root_window_controller.cc:533:7 #8 0x7bc1b5e in ash::WindowTreeHostManager::CloseChildWindows() ash/display/window_tree_host_manager.cc:350:7 #9 0x7c8a720 in ash::Shell::~Shell() ash/shell.cc:767:3 #10 0x7c8dc1d in ash::Shell::~Shell() ash/shell.cc:661:17 #11 0x7c81f98 in ash::Shell::DeleteInstance() ash/shell.cc:210:3 #12 0x6599ab1 in chrome::CloseAsh() chrome/browser/ui/ash/ash_init.cc:104:5 #13 0x1231270c in ChromeBrowserMainParts::PostMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:1850:5 #14 0x56a5a84 in chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:837:3 #15 0x80b952e in content::BrowserMainLoop::ShutdownThreadsAndCleanUp() content/browser/browser_main_loop.cc:987:5 #16 0x88036cc in content::BrowserMainRunnerImpl::Shutdown() content/browser/browser_main_runner.cc:208:7 #17 0x13830cf5 in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:46:3 #18 0x1184b150 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:764:12 #19 0x118482ca in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:15 #20 0x4641e54 in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:277:3 #21 0x4404695 in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:255:3 #22 0x2ecfc4b in policy::DeviceLocalAccountTest::SetUp() chrome/browser/chromeos/policy/device_local_account_browsertest.cc:440:5 #23 0x4ff7ed6 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12 #24 0x4ff7ed6 in testing::Test::Run() testing/gtest/src/gtest.cc:2470 #25 0x4ffa837 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 #26 0x4ffb62a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 #27 0x5010420 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:11 #28 0x500f84b in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 #29 0x500f84b in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 #30 0x45dc506 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 #31 0x45dc506 in base::TestSuite::Run() base/test/test_suite.cc:231 #32 0x288fafa in ChromeBrowserTestSuiteRunner::RunTestSuite(int, char**) chrome/test/base/browser_tests_main.cc:14:12 #33 0x118cb72b in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:499:12 #34 0x42caaa2 in LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**) chrome/test/base/chrome_test_launcher.cc:128:10 #35 0x288f9fc in main chrome/test/base/browser_tests_main.cc:21:10 #36 0x7f125cc1276c in __libc_start_main /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226 .
Message was sent while issue was closed.
sorry for breakage. To whom it may concern, fix is here: https://codereview.chromium.org/1729723003/ -- Evan Stade On Tue, Feb 23, 2016 at 4:22 PM, <shrike@chromium.org> wrote: > A revert of this CL (patchset #9 id:160001) has been created in > https://codereview.chromium.org/1730893002/ by shrike@chromium.org. > > The reason for reverting is: Failure Builder: Linux Chromium OS ASan LSan > Tests > (1) > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > ==12959==ERROR: AddressSanitizer: heap-use-after-free on address > 0x60e000047640 > at pc 0x0000008c9c62 bp 0x7ffc51d4ad70 sp 0x7ffc51d4a530 > WRITE of size 16 at 0x60e000047640 thread T0 (browser_tests) > #0 0x8c9c61 in __asan_memset > (/tmp/runKDXLlt/out/Release/browser_tests+0x8c9c61) > #1 0x7cb68dc in ash::LogoutConfirmationController::OnDialogClosed() > ash/system/chromeos/session/logout_confirmation_controller.cc:83:11 > #2 0x5ce779b in views::Widget::OnNativeWidgetDestroying() > ui/views/widget/widget.cc:1088:3 > #3 0x5d0a61e in OnWindowDestroying > ui/views/widget/native_widget_aura.cc:830:3 > #4 0x5d0a61e in non-virtual thunk to > views::NativeWidgetAura::OnWindowDestroying(aura::Window*) > ui/views/widget/native_widget_aura.cc:828 > #5 0xde3a857 in aura::Window::~Window() ui/aura/window.cc:109:5 > #6 0xde3c02d in aura::Window::~Window() ui/aura/window.cc:102:19 > #7 0x7c41d9c in ash::RootWindowController::CloseChildWindows() > ash/root_window_controller.cc:533:7 > #8 0x7bc1b5e in ash::WindowTreeHostManager::CloseChildWindows() > ash/display/window_tree_host_manager.cc:350:7 > #9 0x7c8a720 in ash::Shell::~Shell() ash/shell.cc:767:3 > #10 0x7c8dc1d in ash::Shell::~Shell() ash/shell.cc:661:17 > #11 0x7c81f98 in ash::Shell::DeleteInstance() ash/shell.cc:210:3 > #12 0x6599ab1 in chrome::CloseAsh() chrome/browser/ui/ash/ash_init.cc:104:5 > #13 0x1231270c in ChromeBrowserMainParts::PostMainMessageLoopRun() > chrome/browser/chrome_browser_main.cc:1850:5 > #14 0x56a5a84 in > chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:837:3 > #15 0x80b952e in content::BrowserMainLoop::ShutdownThreadsAndCleanUp() > content/browser/browser_main_loop.cc:987:5 > #16 0x88036cc in content::BrowserMainRunnerImpl::Shutdown() > content/browser/browser_main_runner.cc:208:7 > #17 0x13830cf5 in content::BrowserMain(content::MainFunctionParams const&) > content/browser/browser_main.cc:46:3 > #18 0x1184b150 in content::ContentMainRunnerImpl::Run() > content/app/content_main_runner.cc:764:12 > #19 0x118482ca in content::ContentMain(content::ContentMainParams const&) > content/app/content_main.cc:19:15 > #20 0x4641e54 in content::BrowserTestBase::SetUp() > content/public/test/browser_test_base.cc:277:3 > #21 0x4404695 in InProcessBrowserTest::SetUp() > chrome/test/base/in_process_browser_test.cc:255:3 > #22 0x2ecfc4b in policy::DeviceLocalAccountTest::SetUp() > chrome/browser/chromeos/policy/device_local_account_browsertest.cc:440:5 > #23 0x4ff7ed6 in HandleExceptionsInMethodIfSupported<testing::Test, void> > testing/gtest/src/gtest.cc:2458:12 > #24 0x4ff7ed6 in testing::Test::Run() testing/gtest/src/gtest.cc:2470 > #25 0x4ffa837 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 > #26 0x4ffb62a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 > #27 0x5010420 in testing::internal::UnitTestImpl::RunAllTests() > testing/gtest/src/gtest.cc:4647:11 > #28 0x500f84b in > HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> > testing/gtest/src/gtest.cc:2458:12 > #29 0x500f84b in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 > #30 0x45dc506 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 > #31 0x45dc506 in base::TestSuite::Run() base/test/test_suite.cc:231 > #32 0x288fafa in ChromeBrowserTestSuiteRunner::RunTestSuite(int, char**) > chrome/test/base/browser_tests_main.cc:14:12 > #33 0x118cb72b in content::LaunchTests(content::TestLauncherDelegate*, int, > int, char**) content/public/test/test_launcher.cc:499:12 > #34 0x42caaa2 in LaunchChromeTests(int, ChromeTestSuiteRunner*, int, > char**) > chrome/test/base/chrome_test_launcher.cc:128:10 > #35 0x288f9fc in main chrome/test/base/browser_tests_main.cc:21:10 > #36 0x7f125cc1276c in __libc_start_main > /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226 > . > > https://codereview.chromium.org/1686433002/ > -- 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. |
