Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478933002:
Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. (Closed)
|
Created:
4 years, 1 month ago by Charlie Reis Modified:
4 years, 1 month ago Reviewers:
Łukasz Anforowicz CC:
chromium-reviews, jam, darin-cc_chromium.org, ncarter (slow), site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix flaky OOPIF tests by waiting for the old RenderFrame to be deleted.
This fixes:
SitePerProcessBrowserTest.CrossSiteIframe
SitePerProcessBrowserTest.NavigateRemoteFrame
SitePerProcessBrowserTest.RFPHDestruction
TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2
TopDocumentIsolationTest.FramesForSitesInHistory
BUG=662550, 629419, 611300, 611344
TEST=Bots stay green.
Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8
Committed: https://crrev.com/29f856890eb1f7c96c3853314a10e782d53cb1eb
Cr-Original-Commit-Position: refs/heads/master@{#430081}
Cr-Commit-Position: refs/heads/master@{#430474}
Patch Set 1 #Patch Set 2 : Fix additional flakiness. #
Total comments: 7
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by creis@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.
creis@chromium.org changed reviewers: + lukasza@chromium.org
lukasza@, can you take a look? I'll need to track down what's happening on the other 2, but these seem to have passed the try jobs. Feel free to CQ if it looks good, or I'm happy to update it on Monday if you have suggestions.
Thanks - lgtm :-)
The CQ bit was checked by lukasza@chromium.org
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.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. ========== to ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081}
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 430081 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2016/11/05 02:04:36, findit-for-me wrote: > FYI: Findit identified this CL at revision 430081 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... Not sure this CL fixed the TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 See https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2479863003/ by mathp@chromium.org. The reason for reverting is: Causing flakiness still: https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests....
Message was sent while issue was closed.
Description was changed from ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ========== to ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ==========
The CQ bit was checked by creis@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...
Sorry for not catching all of them! I did another pass and fixed some more cases in these tests. Łukasz, can you take another look? I've scheduled some extra try jobs on linux_chromium_rel_ng and mac_chromium_asan_rel_ng (where it failed) to see if they can spot anything else. https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:677: RenderFrameDeletedObserver deleted_observer(child->current_frame_host()); The general rule is that we need this around each cross-process navigation, at least when followed by DepictFrameTree (but wouldn't hurt to do always).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for persisting in trying to get rid of the flakiness. https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:653: NavigateFrameToURL(child, http_url); No need for RenderFrameDeletedObserver in-between this NavigateFrameToURL and DepictFrameTree? https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:1735: NavigateFrameToURL(child, http_url); I think I understand no need for RenderFrameDeletedObserver here, because we wait for the *same* child just below. Still - maybe for consistency it might also make sense to have a RenderFrameDeletedObserver here? https://codereview.chromium.org/2478933002/diff/20001/content/browser/top_doc... File content/browser/top_document_isolation_browsertest.cc (right): https://codereview.chromium.org/2478933002/diff/20001/content/browser/top_doc... content/browser/top_document_isolation_browsertest.cc:307: NavigateFrameToURL(root()->child_at(1), c_url); No need for RenderFrameDeletedObserver here (in-between NavigateFrameToURL and DepictFrameTree below)?
Thanks! Do the explanations help clarify it? https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:653: NavigateFrameToURL(child, http_url); On 2016/11/07 22:08:40, Łukasz Anforowicz wrote: > No need for RenderFrameDeletedObserver in-between this NavigateFrameToURL and > DepictFrameTree? This one is same-site, so there's no deletion of a RenderFrame and thus nothing to wait for. https://codereview.chromium.org/2478933002/diff/20001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:1735: NavigateFrameToURL(child, http_url); On 2016/11/07 22:08:40, Łukasz Anforowicz wrote: > I think I understand no need for RenderFrameDeletedObserver here, because we > wait for the *same* child just below. Still - maybe for consistency it might > also make sense to have a RenderFrameDeletedObserver here? We can't, for the same reason as above-- it would just wait forever, since there's no RenderFrame deleted for same-site navigations. https://codereview.chromium.org/2478933002/diff/20001/content/browser/top_doc... File content/browser/top_document_isolation_browsertest.cc (right): https://codereview.chromium.org/2478933002/diff/20001/content/browser/top_doc... content/browser/top_document_isolation_browsertest.cc:307: NavigateFrameToURL(root()->child_at(1), c_url); On 2016/11/07 22:08:40, Łukasz Anforowicz wrote: > No need for RenderFrameDeletedObserver here (in-between NavigateFrameToURL and > DepictFrameTree below)? Interestingly, there's no process swap here, since we're in TDI mode. The main frame doesn't change, and the subframe stays in the "default subframe process" despite going from B to C.
Thanks for the explanations. LGTM.
The CQ bit was checked by creis@chromium.org
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 flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ========== to ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Cr-Commit-Position: refs/heads/master@{#430081} ========== to ========== Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted. This fixes: SitePerProcessBrowserTest.CrossSiteIframe SitePerProcessBrowserTest.NavigateRemoteFrame SitePerProcessBrowserTest.RFPHDestruction TopDocumentIsolationTest.NavigateToSubframeSiteWithPopup2 TopDocumentIsolationTest.FramesForSitesInHistory BUG=662550, 629419, 611300, 611344 TEST=Bots stay green. Committed: https://crrev.com/7a65c0019399f0a7052ab13bc73439acb116adb8 Committed: https://crrev.com/29f856890eb1f7c96c3853314a10e782d53cb1eb Cr-Original-Commit-Position: refs/heads/master@{#430081} Cr-Commit-Position: refs/heads/master@{#430474} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/29f856890eb1f7c96c3853314a10e782d53cb1eb Cr-Commit-Position: refs/heads/master@{#430474} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
