Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2225053002:
Fix tests to behave properly with ISNs/DSNs. (Closed)
|
Created:
4 years, 4 months ago by nasko Modified:
4 years, 3 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix tests to behave properly with ISNs/DSNs.
Currently, there are few tests that behave in a way that is not possible
in regular browsing in full browser. This CL fixes them, so we can enable
some invariant checks.
BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3f85512f193106599ee7f02831b5ef8bed6e0780
Cr-Commit-Position: refs/heads/master@{#410500}
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : Rename test helper method and add TODO. #
Total comments: 4
Patch Set 4 : Fix nits. #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Fix tests to behave properly with ISNs/DSNs. Currently, there are few tests that behave in a way that is not possible in regular browsing in full browser. This CL fixes them, so we can enable some invariant checks. BUG=628677 ========== to ========== Fix tests to behave properly with ISNs/DSNs. Currently, there are few tests that behave in a way that is not possible in regular browsing in full browser. This CL fixes them, so we can enable some invariant checks. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by nasko@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 nasko@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...
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you review this CL for me? It fixes the browser tests to use location.replace and unit tests to copy over the ISN/DSN on SAME_PAGE-like cases. Thanks in advance! Nasko
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sure. I take it the plan is to land this and then turn on the NOTREACHED in a separate CL? Sounds good. One question about the name. https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:483: bool NavigateToURLAndReplace(Shell* shell, const GURL& url) { I'm torn about the name. It sounds a lot like the browser-initiated NavigateToURL case, so it's confusing that it's now renderer-initiated (even though there isn't really a way to do a browser-initiated navigation with replacement in practice). Can we find a different name that avoids the NavigateToURL connotation? And we should definitely update the comment to point out it's renderer-initiated. https://codereview.chromium.org/2225053002/diff/20001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2225053002/diff/20001/content/test/test_rende... content/test/test_render_frame_host.cc:321: // sequence numbers. Simulate this behavior here too. Hmm. This won't be stale when we remove NAVIGATION_TYPE_SAME_PAGE, will it? I suppose Blink will still behave the same, so this is fine to keep.
> I take it the plan is to land this and then turn on the NOTREACHED in a > separate CL? Indeed. Since we've discussed that the tests do something that doesn't happen in practice, I figured this can land separately to improve the tests. https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:483: bool NavigateToURLAndReplace(Shell* shell, const GURL& url) { On 2016/08/08 21:27:11, Charlie Reis wrote: > I'm torn about the name. It sounds a lot like the browser-initiated > NavigateToURL case, so it's confusing that it's now renderer-initiated (even > though there isn't really a way to do a browser-initiated navigation with > replacement in practice). > > Can we find a different name that avoids the NavigateToURL connotation? And we > should definitely update the comment to point out it's renderer-initiated. How about RendererLocationReplace, which follows the naming of the method above (RendererHistoryLenght)? https://codereview.chromium.org/2225053002/diff/20001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2225053002/diff/20001/content/test/test_rende... content/test/test_render_frame_host.cc:321: // sequence numbers. Simulate this behavior here too. On 2016/08/08 21:27:11, Charlie Reis wrote: > Hmm. This won't be stale when we remove NAVIGATION_TYPE_SAME_PAGE, will it? I > suppose Blink will still behave the same, so this is fine to keep. This is more or less relying on Blink keeping its behavior the same. I can put a TODO here to revisit if/when we kill SAME_PAGE classification.
Thanks-- LGTM with nits. https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2225053002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:483: bool NavigateToURLAndReplace(Shell* shell, const GURL& url) { On 2016/08/08 21:57:08, nasko wrote: > On 2016/08/08 21:27:11, Charlie Reis wrote: > > I'm torn about the name. It sounds a lot like the browser-initiated > > NavigateToURL case, so it's confusing that it's now renderer-initiated (even > > though there isn't really a way to do a browser-initiated navigation with > > replacement in practice). > > > > Can we find a different name that avoids the NavigateToURL connotation? And > we > > should definitely update the comment to point out it's renderer-initiated. > > How about RendererLocationReplace, which follows the naming of the method above > (RendererHistoryLenght)? Sounds good, thanks. https://codereview.chromium.org/2225053002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2225053002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:482: // Similar to the ones from content_browser_test_utils. Let's change the comment to: Does a renderer-initiated location.replace navigation to |url|, replacing the current entry. https://codereview.chromium.org/2225053002/diff/40001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2225053002/diff/40001/content/test/test_rende... content/test/test_render_frame_host.cc:323: // codebase, ensure this code is updated to account for it. I don't think we actually need the TODO, since Blink's behavior likely won't change when we remove the classification. (I was just trying to convince myself as much when I mentioned it.) :)
https://codereview.chromium.org/2225053002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2225053002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:482: // Similar to the ones from content_browser_test_utils. On 2016/08/08 22:06:15, Charlie Reis wrote: > Let's change the comment to: > Does a renderer-initiated location.replace navigation to |url|, replacing the > current entry. Argh! Should've caught that one. Thanks! https://codereview.chromium.org/2225053002/diff/40001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/2225053002/diff/40001/content/test/test_rende... content/test/test_render_frame_host.cc:323: // codebase, ensure this code is updated to account for it. On 2016/08/08 22:06:15, Charlie Reis wrote: > I don't think we actually need the TODO, since Blink's behavior likely won't > change when we remove the classification. (I was just trying to convince myself > as much when I mentioned it.) :) Acknowledged.
The CQ bit was checked by nasko@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/2225053002/#ps60001 (title: "Fix nits.")
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 nasko@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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix tests to behave properly with ISNs/DSNs. Currently, there are few tests that behave in a way that is not possible in regular browsing in full browser. This CL fixes them, so we can enable some invariant checks. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix tests to behave properly with ISNs/DSNs. Currently, there are few tests that behave in a way that is not possible in regular browsing in full browser. This CL fixes them, so we can enable some invariant checks. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/3f85512f193106599ee7f02831b5ef8bed6e0780 Cr-Commit-Position: refs/heads/master@{#410500} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3f85512f193106599ee7f02831b5ef8bed6e0780 Cr-Commit-Position: refs/heads/master@{#410500}
Message was sent while issue was closed.
Hi!
I'm concerned about the PageIDUpdatedOnPageReplacement test, its whole point was
in doing a cross-process navigation.
Can you please describe which invariants were violated before this patch?
Currently, if I revert the test locally, it still passes.
Also, what exactly is, as CL description says, not possible in regular browsing
in full browser?
One can easily open a webui page and type location.replace('http://foo.bar/') in
devtools console; this will cause the navigation to go through
FrameHostMsg_OpenURL to chrome::Navigate and finally
NavigationController::LoadURLWithParams will be called with basically same
params. Am I missing something?
Message was sent while issue was closed.
On 2016/09/08 16:16:09, Alexander Semashko wrote: > Hi! Hey! > I'm concerned about the PageIDUpdatedOnPageReplacement test, its whole point was > in doing a cross-process navigation. Looking at the repro steps from https://crbug.com/611679#c2, the new way the test behaves similarly. It instructs the renderer process to issue a "location.replace()" call with the supplied URL. I have missed the bit about cross-process navigation. Why not update the test to an actual URL that will cause the cross-process navigation when it is renderer initiated? > Can you please describe which invariants were violated before this patch? The invariant was that browser initiated navigations don't have should_replace_current_entry set to true. Unfortunately, I can't find in my notes which exact CHECK I tried to add that caused issues with these tests. > Currently, if I revert the test locally, it still passes. Revert what test are you reverting? Or are you reverting the change I made to the test? > Also, what exactly is, as CL description says, not possible in regular browsing > in full browser? Browser initiated navigation with should_replace_current_entry set to true. > One can easily open a webui page and type location.replace('http://foo.bar/') in > devtools console; this will cause the navigation to go through > FrameHostMsg_OpenURL to chrome::Navigate and finally > NavigationController::LoadURLWithParams will be called with basically same > params. Am I missing something? In that case the is_renderer_initiated member of LoadURLParams should be set to true, shouldn't it?
Message was sent while issue was closed.
On 2016/09/09 00:20:20, nasko (slow) wrote: > On 2016/09/08 16:16:09, Alexander Semashko wrote: > > Hi! > > Hey! > > > I'm concerned about the PageIDUpdatedOnPageReplacement test, its whole point > was > > in doing a cross-process navigation. > > Looking at the repro steps from https://crbug.com/611679#c2, the new way the > test behaves similarly. It instructs the renderer process to issue a > "location.replace()" call with the supplied URL. I have missed the bit about > cross-process navigation. Why not update the test to an actual URL that will > cause the cross-process navigation when it is renderer initiated? Ok, I'll look closer to what happens in this test when calling location.replace with changing a webui url to a non-webui url. It should reproduce the bug as well. > > > Can you please describe which invariants were violated before this patch? > > The invariant was that browser initiated navigations don't have > should_replace_current_entry set to true. Unfortunately, I can't find in my > notes which exact CHECK I tried to add that caused issues with these tests. > > > Currently, if I revert the test locally, it still passes. > > Revert what test are you reverting? Or are you reverting the change I made to > the test? > > > Also, what exactly is, as CL description says, not possible in regular > browsing > > in full browser? > > Browser initiated navigation with should_replace_current_entry set to true. Just curious: what are the benefits of enforcing this invariant, noting that calling LoadURLWithParams with is_renderer_initiated==true and should_replace_current_entry==true is absolutely valid? It looks like just a small option that is good to have in some cases, e.g. it might be useful for extensions if there is an api, or for downstream projects. And such navigations could occur in Android Chrome recently (at least, before a flag to initialize renderer without first navigating the WebContents was implemented. > > > One can easily open a webui page and type location.replace('http://foo.bar/') > in > > devtools console; this will cause the navigation to go through > > FrameHostMsg_OpenURL to chrome::Navigate and finally > > NavigationController::LoadURLWithParams will be called with basically same > > params. Am I missing something? > > In that case the is_renderer_initiated member of LoadURLParams should be set to > true, shouldn't it? Yeah, I was somewhat missing the fact the there is is_renderer_initiated flag. When saying "browser-initiated navigation" I meant a navigation issued by browser process (via FrameMsg_Navigate), because it seems that is_renderer_initiated flag does not matter for this test and the related bug. It only needs that a new renderer is created and navigated with should_replace_current_entry==true. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
