Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953513002:
Fix a race between navigation and an interstitial. (Closed)
|
Created:
3 years, 6 months ago by Avi (use Gerrit) Modified:
3 years, 6 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix a race between navigation and an interstitial.
When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that.
BUG=703655
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2953513002
Cr-Commit-Position: refs/heads/master@{#481698}
Committed: https://chromium.googlesource.com/chromium/src/+/668f5234e53dc61582b34a66dda5ef73615db214
Patch Set 1 #
Total comments: 1
Patch Set 2 : with test #
Total comments: 4
Patch Set 3 : docs #Patch Set 4 : reword #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Description was changed from ========== Fix a race between a slow site and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 ========== to ========== Fix a race between a slow site and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by avi@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...
avi@chromium.org changed reviewers: + creis@chromium.org
Charlie, how do you feel about this one?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix a race between a slow site and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix a race between navigation and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Interesting! I didn't even know InterstitialPage::GetIntersitialPage existed. It does seem like a good fit for this. I'm curious if we'd want to move all NavigationControllerDelegate::GetInterstitialPage callers over to this or not. Unfortunately, I'm guessing not-- cases like CanViewSource probably only care if the interstitial is actually visible. That makes me think we want to more carefully document that one of these returns only active interstitials and the other returns pending or active interstitials. Note: There's a handful of interstitial page unit tests in web_contents_impl_unittest.cc, which probably have enough control over timing to let us test this case. Can you add a test for this there? (I added similar one recently in https://codereview.chromium.org/2934353002/diff/20001/content/browser/web_con...) https://codereview.chromium.org/2953513002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2953513002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1883: // If an interstitial page is showing, we want to close it to get back "is showing or is about to show"? (It's worth clarifying why we're using this one instead of delegate_->GetInterstitialPage() if we can't replace all the uses.)
On 2017/06/21 06:02:31, Charlie Reis (slow) wrote: > Interesting! I didn't even know InterstitialPage::GetIntersitialPage existed. > It does seem like a good fit for this. You can look up interstitial pages via WebContentses in two ways, and those mappings are updated at different times. I do see this as a design flaw, one that I'm taking advantage of here. Given that Emily is going to be reworking interstitials to be less broken, I'm OK with taking advantage of this here. I agree with your two points: documentation at that moment as to why we're using one rather than the other, and testing. Will work on that later today.
The CQ bit was checked by avi@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.
Charlie, I have a test now. PTAL.
Thanks, LGTM! Can you also update the comments on the declarations of WebContents::GetInterstitialPage and InterstitialPage::GetInterstitialPage, to indicate that they will or will not return an interstitial after it's been created and before it commits its page and becomes visible? https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1886: // null if the interstitial was shown but not yet visible while nit: s/shown/created/, perhaps?
The CQ bit was checked by avi@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...
Charlie, ptal at the docs. https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1886: // null if the interstitial was shown but not yet visible while On 2017/06/22 16:19:38, Charlie Reis (slow) wrote: > nit: s/shown/created/, perhaps? I'm talking about the range in which WebContents::GetInterstitialPage and InterstitialPage::GetInterstitialPage differ. It's precisely the time from when the interstitial was told to "show" through the time the interstitial is visible that GetInterstitialPage returns null but GetInterstitialPage returns the interstitial.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks-- the extra declaration comments help a lot! LGTM with nit. https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1886: // null if the interstitial was shown but not yet visible while On 2017/06/22 18:08:51, Avi (ping after 24h) wrote: > On 2017/06/22 16:19:38, Charlie Reis (slow) wrote: > > nit: s/shown/created/, perhaps? > > I'm talking about the range in which WebContents::GetInterstitialPage and > InterstitialPage::GetInterstitialPage differ. It's precisely the time from when > the interstitial was told to "show" through the time the interstitial is visible > that GetInterstitialPage returns null but GetInterstitialPage returns the > interstitial. Ok. Can you rephrase to say "if the interstitial's Show() method was called but it is not yet visible, while..."? I'm finding "shown but not visible" hard to understand without clarifying that one of them is a method.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2953513002/#ps60001 (title: "reword")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2953513002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1886: // null if the interstitial was shown but not yet visible while On 2017/06/22 21:06:00, Charlie Reis (slow) wrote: > On 2017/06/22 18:08:51, Avi (ping after 24h) wrote: > > On 2017/06/22 16:19:38, Charlie Reis (slow) wrote: > > > nit: s/shown/created/, perhaps? > > > > I'm talking about the range in which WebContents::GetInterstitialPage and > > InterstitialPage::GetInterstitialPage differ. It's precisely the time from > when > > the interstitial was told to "show" through the time the interstitial is > visible > > that GetInterstitialPage returns null but GetInterstitialPage returns the > > interstitial. > > Ok. Can you rephrase to say "if the interstitial's Show() method was called but > it is not yet visible, while..."? I'm finding "shown but not visible" hard to > understand without clarifying that one of them is a method. I rewrote it a few times, and I'm now much happier with the phrasing. Thank you for the push.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498166087564840,
"parent_rev": "3cfaf63ef722488442e21321e4f84baa9eb8c4d4", "commit_rev":
"668f5234e53dc61582b34a66dda5ef73615db214"}
Message was sent while issue was closed.
Description was changed from ========== Fix a race between navigation and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix a race between navigation and an interstitial. When early-exiting NavigationControllerImpl::NavigateToPendingEntry, the interstitial page may have been created but not yet attached itself to the WebContents. In that case, because asking the WebContents for the interstitial won't work, ask the interstitial pages if any of them are attached to the desired WebContents. Even before attaching to the WebContents, the interstitial pages know that. BUG=703655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2953513002 Cr-Commit-Position: refs/heads/master@{#481698} Committed: https://chromium.googlesource.com/chromium/src/+/668f5234e53dc61582b34a66dda5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/668f5234e53dc61582b34a66dda5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
