Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918293002:
Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest (Closed)
DescriptionEnsure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest
Currently they seem to get destroyed by luck. Chrome does a lot of work
in TearDown, and InProcessBrowserTests spins run loops enough times,
that the ExtensionHosts are destroyed before the Profile is. For some
tests, the extension host tear down only starts when the parent browser
window is closed. But, for extension popups, there's no explicit wait.
This means that Extension Popup tests can flakily trigger DCHECKs in
ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official
releases, DestroyProfileWhenAppropriate has a codepath to clean up.
Add a class to the test harness that ensures
NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before
continuing to browser window shutdown. This makes the failures assured
rather than flaky. Then add explicit calls to close the popup to the
tests that are missing it.
BUG=729476
Review-Url: https://codereview.chromium.org/2918293002
Cr-Commit-Position: refs/heads/master@{#477083}
Committed: https://chromium.googlesource.com/chromium/src/+/910f537829fe4483c965d4c16dcfd54e549cffbc
Patch Set 1 #Patch Set 2 : fix one #Patch Set 3 : clear upstream CL #
Total comments: 3
Messages
Total messages: 21 (16 generated)
The CQ bit was checked by tapted@chromium.org 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_...)
Description was changed from ========== Ensure all BrowserAction ExtensionHosts are closed in BrowserActionInteractiveTest BUG=729476 ========== to ========== Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest Currently they seem to get destroyed by luck. Chrome does a lot of work in TearDown, and InProcessBrowserTests spins run loops enough times, that the ExtensionHosts are destroyed before the Profile is. For some tests, the extension host tear down only starts when the parent browser window is closed. But, for extension popups, there's no explicit wait. This means that Extension Popup tests can flakily trigger DCHECKs in ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official releases, DestroyProfileWhenAppropriate has a codepath to clean up. To fix, add a class to the test harness that ensures NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before continuing to browser window shutdown. This makes the failures assured rather than flaky. Then add explicit calls to close the popup to the tests that are missing it. BUG=729476 ==========
tapted@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
The CQ bit was checked by tapted@chromium.org 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 checked by tapted@chromium.org 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.
Description was changed from ========== Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest Currently they seem to get destroyed by luck. Chrome does a lot of work in TearDown, and InProcessBrowserTests spins run loops enough times, that the ExtensionHosts are destroyed before the Profile is. For some tests, the extension host tear down only starts when the parent browser window is closed. But, for extension popups, there's no explicit wait. This means that Extension Popup tests can flakily trigger DCHECKs in ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official releases, DestroyProfileWhenAppropriate has a codepath to clean up. To fix, add a class to the test harness that ensures NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before continuing to browser window shutdown. This makes the failures assured rather than flaky. Then add explicit calls to close the popup to the tests that are missing it. BUG=729476 ========== to ========== Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest Currently they seem to get destroyed by luck. Chrome does a lot of work in TearDown, and InProcessBrowserTests spins run loops enough times, that the ExtensionHosts are destroyed before the Profile is. For some tests, the extension host tear down only starts when the parent browser window is closed. But, for extension popups, there's no explicit wait. This means that Extension Popup tests can flakily trigger DCHECKs in ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official releases, DestroyProfileWhenAppropriate has a codepath to clean up. Add a class to the test harness that ensures NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before continuing to browser window shutdown. This makes the failures assured rather than flaky. Then add explicit calls to close the popup to the tests that are missing it. BUG=729476 ==========
Hi Devlin, please take a look. This is blocking http://crrev.com/2909053002/ from landing. +dullweber cc fyi
lgtm I am a little worried that this is going to mask a bug, because users won't always close popups before closing the browser, but I don't have much better suggestions at the moment. https://codereview.chromium.org/2918293002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc (right): https://codereview.chromium.org/2918293002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:43: // BrowserProcessImpl::StartTearDown(). TODO(tapted): The existence of this I think I'd agree with this. What happens if a user closes a browser with an open popup? Does it trigger the asserts? https://codereview.chromium.org/2918293002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:62: FROM_HERE, quit_closure_, TestTimeouts::action_timeout()); Interesting! I don't think I've seen this before.
https://codereview.chromium.org/2918293002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc (right): https://codereview.chromium.org/2918293002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:43: // BrowserProcessImpl::StartTearDown(). TODO(tapted): The existence of this On 2017/06/05 14:22:11, Devlin wrote: > I think I'd agree with this. What happens if a user closes a browser with an > open popup? Does it trigger the asserts? The code in ProfileDestroyer::DestroyProfileWhenAppropriate(..) has a supported path for destroying incognito renderer hosts before the Profile is destroyed. So for users everything should work fine - extension hosts just fall into that path. There's a comment around the DCHECK saying ~"this is bad maybe you're leaking hosts" but with this code added we are checking that there is no leak.
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496696397328430,
"parent_rev": "5b10f8f6e185160f4b30d279ad7feb913cf5f02c", "commit_rev":
"910f537829fe4483c965d4c16dcfd54e549cffbc"}
Message was sent while issue was closed.
Description was changed from ========== Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest Currently they seem to get destroyed by luck. Chrome does a lot of work in TearDown, and InProcessBrowserTests spins run loops enough times, that the ExtensionHosts are destroyed before the Profile is. For some tests, the extension host tear down only starts when the parent browser window is closed. But, for extension popups, there's no explicit wait. This means that Extension Popup tests can flakily trigger DCHECKs in ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official releases, DestroyProfileWhenAppropriate has a codepath to clean up. Add a class to the test harness that ensures NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before continuing to browser window shutdown. This makes the failures assured rather than flaky. Then add explicit calls to close the popup to the tests that are missing it. BUG=729476 ========== to ========== Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest Currently they seem to get destroyed by luck. Chrome does a lot of work in TearDown, and InProcessBrowserTests spins run loops enough times, that the ExtensionHosts are destroyed before the Profile is. For some tests, the extension host tear down only starts when the parent browser window is closed. But, for extension popups, there's no explicit wait. This means that Extension Popup tests can flakily trigger DCHECKs in ProfileDestroyer::DestroyProfileWhenAppropriate(..). But, for official releases, DestroyProfileWhenAppropriate has a codepath to clean up. Add a class to the test harness that ensures NOTIFICATION_EXTENSION_HOST_CREATED/DESTOYED are balanced before continuing to browser window shutdown. This makes the failures assured rather than flaky. Then add explicit calls to close the popup to the tests that are missing it. BUG=729476 Review-Url: https://codereview.chromium.org/2918293002 Cr-Commit-Position: refs/heads/master@{#477083} Committed: https://chromium.googlesource.com/chromium/src/+/910f537829fe4483c965d4c16dcf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/910f537829fe4483c965d4c16dcf... |
