Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474783002:
Fix memory leak for extension uninstall dialog. (Closed)
DescriptionFix memory leak for extension uninstall dialog
test=Add Test coverage. Pass browser test.
BUG=661373
Committed: https://crrev.com/949dbeec49cba970a01fbcc4cc99edb4598bbe7a
Cr-Commit-Position: refs/heads/master@{#432320}
Patch Set 1 #Patch Set 2 : cheat presubmit to run trybots #Patch Set 3 : Add test coverage. #
Total comments: 11
Patch Set 4 : Address Devlin's comments #Patch Set 5 : Fix memory leak for extension uninstall dialog. #Patch Set 6 : Address Oshima's comments. #
Total comments: 4
Patch Set 7 : Nits. #
Total comments: 3
Patch Set 8 : Remove a test for MAC. #
Total comments: 4
Patch Set 9 : Mac test coverage. #Patch Set 10 : Address Trent's comments. #
Total comments: 1
Patch Set 11 : Address Mike's last minute comment. #Patch Set 12 : Rebase #
Messages
Total messages: 123 (101 generated)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lgcheng@google.com
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] Fix memory leak for extension uninstall dialog. test BUG= ========== to ========== [WIP] Fix memory leak for extension uninstall dialog. Will not commit. test BUG= ==========
Description was changed from ========== [WIP] Fix memory leak for extension uninstall dialog. Will not commit. test BUG= ========== to ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit. test BUG= ==========
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 lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit. test BUG= ========== to ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit until clean up. test BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit until clean up. test BUG= ========== to ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit until clean up. test BUG=661373 ==========
lgcheng@google.com changed reviewers: + lhchavez@chromium.org, rdcronin@google.com, rdevlin.cronin@chromium.org
Hi Devlin, Sorry to send this out before I clean this up. But I have problems. Would you PTAL at the comment I have added? cc +Luis https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:72: &delegate)); This test is still working with what I have changed. The uninstall_dialog is killed because the extension is not added to ExtensionRegistry, not because the parent is killed. Which means the test does not cover what it claims. If we fix this, I would imagine that this test will fail on Mac for the same reason that the test I added below fails. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:93: extensions::ExtensionRegistry* registry = Add extension to ExtensionRegistry so that uninstall_dialog flow works correctly https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... The logs show that attempt works as: [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(122)] 0x28eaf45255d0 Dialog Created [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(186)] 0x28eaf434aba0 Dialog View Created [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(198)] 0x28eaf434aba0 Dialog View Died [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(131)] 0x28eaf45255d0 Dialog Died But I am not sure this is the neat way to do. Any suggestions? https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:113: The test fails on Mac because on Mac it has different implementation. https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/exten... It does not care about parent window's life at all and leave that to the platform. So the browser()->window()->Close() can not kill the dialog thus test fails due to timeout. Shall we only enable this test when the platform is not Mac? (I am not sure if the memory leak happens on Mac too since I am not familiar with that platform. Although it claims "dialog blocks the page from navigating away and destroying the dialog, so there's no way for the dialog to outlive its delegate." But I would imagine that user can still force stop Chrome browser somewhere leaving the extension_uninstall_dialog hang forever.)
https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:147: OnDialogClosed(CLOSE_ACTION_CANCELED); It seems like there's a potential that this could be called twice now, since the dialog view is eventually destructed after e.g. hitting accept. In the tests, this won't necessarily fail, but it would be bad in production (where we have lifetime issues and real consequences calling multiple times). We should a) guard this here and b) maybe put in a DCHECK ExtensionUninstallDialog::OnDialogClosed to make sure we don't call twice. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:72: &delegate)); On 2016/11/03 22:30:50, lgcheng wrote: > This test is still working with what I have changed. The uninstall_dialog is > killed because the extension is not added to ExtensionRegistry, not because the > parent is killed. Which means the test does not cover what it claims. > > If we fix this, I would imagine that this test will fail on Mac for the same > reason that the test I added below fails. Yeah, we should make sure the extension is added. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:93: extensions::ExtensionRegistry* registry = On 2016/11/03 22:30:50, lgcheng wrote: > Add extension to ExtensionRegistry so that uninstall_dialog flow works correctly > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... > > The logs show that attempt works as: > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(122)] > 0x28eaf45255d0 Dialog Created > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(186)] > 0x28eaf434aba0 Dialog View Created > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(198)] > 0x28eaf434aba0 Dialog View Died > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(131)] > 0x28eaf45255d0 Dialog Died > > > > But I am not sure this is the neat way to do. Any suggestions? Doing ExtensionSystem::Get(browser()->profile())->extension_service()->AddExtension(extension.get()); is probably more right - it adds the extension in the normal way and updates any other listeners. (Adding the extension directly to the registry would probably be perfect for a unit test, but might mess us up in a browser test.) https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:98: DCHECK(registry->AddEnabled(extension)); moot with the above comment, but prefer ASSERT_TRUE in cases like this.
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] Fix memory leak for extension uninstall dialog. Do not submit until clean up. test BUG=661373 ========== to ========== Fix memory leak for extension uninstall dialog test=Add Test coverage. Pass browser test. BUG=661373 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
oshima@chromium.org changed reviewers: + oshima@chromium.org
looks like there is a bug in ConstrainedWindowView https://cs.chromium.org/chromium/src/components/constrained_window/constraine... It should listen to OnWidgetDestorying instead of OnWidgetClosing and that's why WidgetModalDialogHostObnserverView is leaking.
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi guys, Would you PTAL when you have a moment? Hi Devlin@, Address your comments in the update. Sorry for updating so late but we have onsite/offsite events this week, Hi Oshima@, Fix browser_test memory leak with you suggestion. Thanks! Adding msw@ for OWNER review. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:147: OnDialogClosed(CLOSE_ACTION_CANCELED); On 2016/11/05 06:26:18, Devlin (slow) wrote: > It seems like there's a potential that this could be called twice now, since the > dialog view is eventually destructed after e.g. hitting accept. In the tests, > this won't necessarily fail, but it would be bad in production (where we have > lifetime issues and real consequences calling multiple times). We should a) > guard this here and b) maybe put in a DCHECK > ExtensionUninstallDialog::OnDialogClosed to make sure we don't call twice. Thanks for pointing out the potential issue. But after re-examine more on details of implementation, I think it's safe since in the deconstructor of ExtensionUninstallDialogDelegateView there is a check if (dialog_) dialog_->DialogDelegateDestroyed(); If the view is closed due to accept or cancel, ExtensionUninstallDialogViews::DialogAccepted or ExtensionUninstallDialogViews::DialogCanceled() will be called, in which view_->DialogDestroyed() { dialog_ = NULL} is called and guarantees dialog_->DialogDelegateDestroyed() will not be called in this situation. But I agree to add debug mode checks for this. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:72: &delegate)); On 2016/11/05 06:26:18, Devlin (slow) wrote: > On 2016/11/03 22:30:50, lgcheng wrote: > > This test is still working with what I have changed. The uninstall_dialog is > > killed because the extension is not added to ExtensionRegistry, not because > the > > parent is killed. Which means the test does not cover what it claims. > > > > If we fix this, I would imagine that this test will fail on Mac for the same > > reason that the test I added below fails. > > Yeah, we should make sure the extension is added. Done. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:93: extensions::ExtensionRegistry* registry = On 2016/11/05 06:26:18, Devlin (slow) wrote: > On 2016/11/03 22:30:50, lgcheng wrote: > > Add extension to ExtensionRegistry so that uninstall_dialog flow works > correctly > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... > > > > The logs show that attempt works as: > > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(122)] > > 0x28eaf45255d0 Dialog Created > > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(186)] > > 0x28eaf434aba0 Dialog View Created > > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(198)] > > 0x28eaf434aba0 Dialog View Died > > [28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(131)] > > 0x28eaf45255d0 Dialog Died > > > > > > > > But I am not sure this is the neat way to do. Any suggestions? > > Doing > ExtensionSystem::Get(browser()->profile())->extension_service()->AddExtension(extension.get()); > is probably more right - it adds the extension in the normal way and updates any > other listeners. (Adding the extension directly to the registry would probably > be perfect for a unit test, but might mess us up in a browser test.) Done. https://codereview.chromium.org/2474783002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:98: DCHECK(registry->AddEnabled(extension)); On 2016/11/05 06:26:18, Devlin (slow) wrote: > moot with the above comment, but prefer ASSERT_TRUE in cases like this. Done.
lgcheng@google.com changed reviewers: + msw@chromium.org
Adding msw@ for OWNER's review.
This looks fine to me, but I'll let msw@ stamp it, since he's the views owner. :) https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:146: view_ = NULL; nit: prefer nullptr in new code. https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:59: #if !defined(OS_MACOSX) By default, we don't use views on Mac. The only time we do is when MacViews are enabled - and if that's the case, it seems like we should have this test? I'll let a views owner decide. In either case, I'd think we shouldn't be commenting out an existing test.
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:146: view_ = NULL; On 2016/11/14 17:49:12, Devlin wrote: > nit: prefer nullptr in new code. Done. https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:59: #if !defined(OS_MACOSX) On 2016/11/14 17:49:12, Devlin wrote: > By default, we don't use views on Mac. The only time we do is when MacViews are > enabled - and if that's the case, it seems like we should have this test? I'll > let a views owner decide. > > In either case, I'd think we shouldn't be commenting out an existing test. Since this existing test is to verify dialog is destroyed before view is created, it will work on Mac since dialog view related logic will not affect as view is not created yet. I'll stay with your suggestion and keep this test for Mac. And I'll run trybots to verify this test.
lgtm with a nit https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:96: CreateBrowser(browser()->profile()); nit: use ScopedKeepAlive w/LEAKED_UNINSTALL_VIEW instead? (here and above)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Mike, Thanks for your comments! But I make a further change in the patch. Would you PTAL? Thanks! https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:96: CreateBrowser(browser()->profile()); On 2016/11/14 20:04:27, msw wrote: > nit: use ScopedKeepAlive w/LEAKED_UNINSTALL_VIEW instead? (here and above) It seems CreateBrowser(browser()->profile()) is not needed for this test and the test above. I remove it. If I use ScopedKeepAlive, I need manually reset it after browser()->window()->Close(), otherwise Check will fail(FATAL:browser_process_impl.cc(1330)] Check failed: base::MessageLoop::current()->is_running(). ) in deconstructor of ScopedKeepAlive after run_loop.QuitClosure(). But reset ScopedKeepAlive right after browser()->window()->Close() looks strange to me. Correct me if I am wrong. https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:59: #if !defined(OS_MACOSX) This test won't run on MAC. 1. It run on MAC before because the test extension was not added to extension system and test exit with extension not found https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... 2. After I add the extension to extension system, this test fails because all the logic that tracks parent window lies in https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... Parent window killed before view is created: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... Parent window killed after view is created: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... However above mentioned implementation does not run on Mac, which means Mac does not track parent window lifetime. Mac's implementation. https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/exten... Unfortunately I have to exclude this test for Mac.
msw@chromium.org changed reviewers: + tapted@chromium.org
+Trent for Mac Views testing help. https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:96: CreateBrowser(browser()->profile()); On 2016/11/14 23:51:41, lgcheng wrote: > On 2016/11/14 20:04:27, msw wrote: > > nit: use ScopedKeepAlive w/LEAKED_UNINSTALL_VIEW instead? (here and above) > > It seems CreateBrowser(browser()->profile()) is not needed for this test and the > test above. I remove it. > If I use ScopedKeepAlive, I need manually reset it after > browser()->window()->Close(), otherwise Check will > fail(FATAL:browser_process_impl.cc(1330)] Check failed: > base::MessageLoop::current()->is_running(). ) in deconstructor of > ScopedKeepAlive after run_loop.QuitClosure(). But reset ScopedKeepAlive right > after browser()->window()->Close() looks strange to me. Correct me if I am > wrong. If neither is needed; that's great. https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:57: // Following test does not apply on MACOSX. MACOSX have its own implementation grammar nits: "The following tests do not apply to Mac's Cocoa UI." https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:59: #if !defined(OS_MACOSX) On 2016/11/14 23:51:41, lgcheng wrote: > This test won't run on MAC. > > 1. It run on MAC before because the test extension was not added to extension > system and test exit with extension not found > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... > > 2. After I add the extension to extension system, this test fails because all > the logic that tracks parent window lies in > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > Parent window killed before view is created: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > Parent window killed after view is created: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > However above mentioned implementation does not run on Mac, which means Mac does > not track parent window lifetime. > Mac's implementation. > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/exten... > > Unfortunately I have to exclude this test for Mac. extension_uninstall_dialog_cocoa should not be used if the mac port is using views dialogs... try enabling this test for Mac Views; ping tapted for help.
https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:59: #if !defined(OS_MACOSX) On 2016/11/15 00:09:32, msw wrote: > On 2016/11/14 23:51:41, lgcheng wrote: > > This test won't run on MAC. > > > > 1. It run on MAC before because the test extension was not added to extension > > system and test exit with extension not found > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin... > > > > 2. After I add the extension to extension system, this test fails because all > > the logic that tracks parent window lies in > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > > > Parent window killed before view is created: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > > > Parent window killed after view is created: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/exten... > > > > However above mentioned implementation does not run on Mac, which means Mac > does > > not track parent window lifetime. > > Mac's implementation. > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/exten... > > > > Unfortunately I have to exclude this test for Mac. > > extension_uninstall_dialog_cocoa should not be used if the mac port is using > views dialogs... try enabling this test for Mac Views; ping tapted for help. Try adding #if defined(OS_MACOSX) #include "chrome/browser/ui/browser_dialogs.h" #endif void SetUpCommandLine(base::CommandLine* command_line) override { #if defined(OS_MACOSX) command_line->AppendSwitchASCII(switches::kEnableFeatures, kMacViewsWebUIDialogs.name); #endif } IF that doesn't work, the file should be moved in the build file to the right configuration, but we should instead just run these tests with kMacViewsWebUIDialogs so that we get proper test coverage. (see e.g. https://cs.chromium.org/chromium/src/content/browser/media/encrypted_media_br... for another test that uses this pattern)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lgcheng@google.com
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Ah, I see the problem now. Thank you for giving it a try (and sorry for the bad
advice).
The issue in this case is that we've plumbed through the extension *install*
dialog. However, the extension *uninstall* dialog on Mac has always been really
basic -- just an NSAlert (basically a fancy ::MessageBox(..)). So the best
approach here would be to adjust chrome/test/BUILD.gn:
extension_uninstall_dialog_view_browsertest.cc is currently controlled `if
(toolkit_views)`, but it needs to be controlled by `if (!is_mac ||
mac_views_browser) {`
I'll work on plumbing through the uninstall dialog for MacViews so that it is
properly consistent with the other dialogs when they switch to Harmony.
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/15 04:28:18, tapted wrote:
> Ah, I see the problem now. Thank you for giving it a try (and sorry for the
bad
> advice).
>
> The issue in this case is that we've plumbed through the extension *install*
> dialog. However, the extension *uninstall* dialog on Mac has always been
really
> basic -- just an NSAlert (basically a fancy ::MessageBox(..)). So the best
> approach here would be to adjust chrome/test/BUILD.gn:
>
> extension_uninstall_dialog_view_browsertest.cc is currently controlled `if
> (toolkit_views)`, but it needs to be controlled by `if (!is_mac ||
> mac_views_browser) {`
>
> I'll work on plumbing through the uninstall dialog for MacViews so that it is
> properly consistent with the other dialogs when they switch to Harmony.
Thanks Trent for your suggestions!
It works for now. Let me know if there is any further issue.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2474783002/#ps260001 (title: "Address Trent's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nice; thanks! lgtm with a very optional nit (already in cq) https://codereview.chromium.org/2474783002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2474783002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:68: nit: remove blank line
The CQ bit was unchecked by lgcheng@google.com
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2474783002/#ps280001 (title: "Address Mike's last minute comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2474783002/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix memory leak for extension uninstall dialog test=Add Test coverage. Pass browser test. BUG=661373 ========== to ========== Fix memory leak for extension uninstall dialog test=Add Test coverage. Pass browser test. BUG=661373 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Fix memory leak for extension uninstall dialog test=Add Test coverage. Pass browser test. BUG=661373 ========== to ========== Fix memory leak for extension uninstall dialog test=Add Test coverage. Pass browser test. BUG=661373 Committed: https://crrev.com/949dbeec49cba970a01fbcc4cc99edb4598bbe7a Cr-Commit-Position: refs/heads/master@{#432320} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/949dbeec49cba970a01fbcc4cc99edb4598bbe7a Cr-Commit-Position: refs/heads/master@{#432320} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
