Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1802383004:
Fix in-page navigation after LoadDataWithBaseURL (Closed)
|
Created:
4 years, 9 months ago by boliu Modified:
4 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix in-page navigation after LoadDataWithBaseURL
This is a follow up fix to r361481. After r361481 additional info
about a LoadDataWithBaseURL is stored in DocumentState.
However, if there is an in-page fragment navigation, this additional
info is just thrown away, causing incorrect behavior. Fix this by not
updating this additional info for in-page navigations.
Note this is NOT a fix for crbug.com/561034, which describes a similar
problem, but on the browser side.
BUG=594611
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ac512fd1188a7bd1e1e752bd46990acd72b6bdb6
Cr-Commit-Position: refs/heads/master@{#381588}
Patch Set 1 #Patch Set 2 : browser test #
Total comments: 7
Patch Set 3 : review #Patch Set 4 : test expectations #Patch Set 5 : Fix test for LoadDataAsStringWithBaseURL on android #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== Fix in-page navigation after LoadDataWithBaseURL BUG=561034 ========== to ========== Fix in-page navigation after LoadDataWithBaseURL BUG=594611 ==========
Description was changed from ========== Fix in-page navigation after LoadDataWithBaseURL BUG=594611 ========== to ========== Fix in-page navigation after LoadDataWithBaseURL BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802383004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802383004/20001
Description was changed from ========== Fix in-page navigation after LoadDataWithBaseURL BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix in-page navigation after LoadDataWithBaseURL This is a follow up fix to r361481. After r361481 additional info about a LoadDataWithBaseURL is stored in DocumentState. However, if there is an in-page fragment navigation, this additional info is just thrown away, causing incorrect behavior. Fix this by not updating this additional info for in-page navigations. Note this is NOT a fix for crbug.com/561034, which describes a similar problem, but on the browser side. BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
boliu@chromium.org changed reviewers: + creis@chromium.org
PTAL I think the crash stack in the bug plus the test should be enough information about the issue.
On 2016/03/16 02:11:11, boliu wrote: > PTAL > > I think the crash stack in the bug plus the test should be enough information > about the issue. ping! A webview m49 respin is waiting for this fix..
Sigh. This feature continues to be a source of fun, eh? :) So avoiding the update means the DocumentState retains its earlier data URL and allows is_same_origin to be true in IsURLInPageNavigation? Ok, LGTM with nits. https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:229: // Performance a fragment navigation using a javascript: URL. Perform? https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:232: EXPECT_EQ(2, controller.GetEntryCount()); Please add some expectations about what the various URLs are in this case (like lines 198-202 above), since it's not clear from reading the code. https://codereview.chromium.org/1802383004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1802383004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5903: bool was_navigate_within_same_page) { nit: was_within_same_page (to match NavigationStateImpl's naming). Let's update the .h file as well. https://codereview.chromium.org/1802383004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5915: if (!was_navigate_within_same_page) { This needs a comment about why in-page navigations shouldn't go through this block (e.g., don't clear the data URL on the DocumentState if one was present).
On 2016/03/16 19:30:34, Charlie Reis wrote: > Sigh. This feature continues to be a source of fun, eh? :) Sadly yes :( > > So avoiding the update means the DocumentState retains its earlier data URL and > allows is_same_origin to be true in IsURLInPageNavigation? Yep > Ok, LGTM with nits. > https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:232: EXPECT_EQ(2, controller.GetEntryCount()); On 2016/03/16 19:30:34, Charlie Reis wrote: > Please add some expectations about what the various URLs are in this case (like > lines 198-202 above), since it's not clear from reading the code. On trunk these values are Base url: empty virtual url: the data url history url for data url: empty I think these are bogus values due to crbug.com/561034, which this CL is *not* fixing. Do you prefer I EXPECT these, but add a comment that about crbug.com/561034 then? Last time this came up, we just removed the whole section in the test.
https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:232: EXPECT_EQ(2, controller.GetEntryCount()); On 2016/03/16 20:18:21, boliu wrote: > On 2016/03/16 19:30:34, Charlie Reis wrote: > > Please add some expectations about what the various URLs are in this case > (like > > lines 198-202 above), since it's not clear from reading the code. > > > On trunk these values are > > Base url: empty > virtual url: the data url > history url for data url: empty > > I think these are bogus values due to crbug.com/561034, which this CL is *not* > fixing. Do you prefer I EXPECT these, but add a comment that about > crbug.com/561034 then? > > Last time this came up, we just removed the whole section in the test. I uploaded PS3 with all the other comments addressed. Let me know how you feel about this one.
https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1802383004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:232: EXPECT_EQ(2, controller.GetEntryCount()); On 2016/03/16 20:26:07, boliu wrote: > On 2016/03/16 20:18:21, boliu wrote: > > On 2016/03/16 19:30:34, Charlie Reis wrote: > > > Please add some expectations about what the various URLs are in this case > > (like > > > lines 198-202 above), since it's not clear from reading the code. > > > > > > On trunk these values are > > > > Base url: empty > > virtual url: the data url > > history url for data url: empty > > > > I think these are bogus values due to crbug.com/561034, which this CL is *not* > > fixing. Do you prefer I EXPECT these, but add a comment that about > > crbug.com/561034 then? > > > > Last time this came up, we just removed the whole section in the test. > > I uploaded PS3 with all the other comments addressed. > > Let me know how you feel about this one. Err, wait, these are just the expectations for a js navigation, so actually exactly the same as lines 198-202. So adding the expectations, along with a comment mentioning crbug.com/561034
The CQ bit was checked by boliu@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/1802383004/#ps60001 (title: "test expectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802383004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802383004/60001
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by boliu@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/1802383004/#ps80001 (title: "Fix test for LoadDataAsStringWithBaseURL on android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802383004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802383004/80001
Message was sent while issue was closed.
Description was changed from ========== Fix in-page navigation after LoadDataWithBaseURL This is a follow up fix to r361481. After r361481 additional info about a LoadDataWithBaseURL is stored in DocumentState. However, if there is an in-page fragment navigation, this additional info is just thrown away, causing incorrect behavior. Fix this by not updating this additional info for in-page navigations. Note this is NOT a fix for crbug.com/561034, which describes a similar problem, but on the browser side. BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix in-page navigation after LoadDataWithBaseURL This is a follow up fix to r361481. After r361481 additional info about a LoadDataWithBaseURL is stored in DocumentState. However, if there is an in-page fragment navigation, this additional info is just thrown away, causing incorrect behavior. Fix this by not updating this additional info for in-page navigations. Note this is NOT a fix for crbug.com/561034, which describes a similar problem, but on the browser side. BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix in-page navigation after LoadDataWithBaseURL This is a follow up fix to r361481. After r361481 additional info about a LoadDataWithBaseURL is stored in DocumentState. However, if there is an in-page fragment navigation, this additional info is just thrown away, causing incorrect behavior. Fix this by not updating this additional info for in-page navigations. Note this is NOT a fix for crbug.com/561034, which describes a similar problem, but on the browser side. BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix in-page navigation after LoadDataWithBaseURL This is a follow up fix to r361481. After r361481 additional info about a LoadDataWithBaseURL is stored in DocumentState. However, if there is an in-page fragment navigation, this additional info is just thrown away, causing incorrect behavior. Fix this by not updating this additional info for in-page navigations. Note this is NOT a fix for crbug.com/561034, which describes a similar problem, but on the browser side. BUG=594611 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ac512fd1188a7bd1e1e752bd46990acd72b6bdb6 Cr-Commit-Position: refs/heads/master@{#381588} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac512fd1188a7bd1e1e752bd46990acd72b6bdb6 Cr-Commit-Position: refs/heads/master@{#381588} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
