Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452443002:
Drop navigations to NavigationEntry with invalid virtual URLs. (Closed)
DescriptionDrop navigations to NavigationEntry with invalid virtual URLs.
BUG=657720
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/e4ebe078840e65d673722e94f8251b334030b5e8
Cr-Commit-Position: refs/heads/master@{#428056}
Patch Set 1 #Patch Set 2 : Drop navigations to NavigationEntry with invalid virtual URLs. #
Total comments: 8
Patch Set 3 : Fixes based on code review. #
Total comments: 2
Patch Set 4 : Fix comment. #
Messages
Total messages: 29 (20 generated)
Description was changed from ========== Don't create pending NavigationEntry for invalid URLs. BUG=657720 ========== to ========== Don't create pending NavigationEntry for invalid URLs. BUG=657720 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Description was changed from ========== Don't create pending NavigationEntry for invalid URLs. BUG=657720 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Drop navigations to NavigationEntry with invalid virtual URLs. BUG=657720 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...
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, This is a potential fix for the issue described in 657720. We already drop navigations to invalid URLs, so it doesn't seem too far fetched to drop navigations where the virtual URL is invalid too. What do you think? Thanks in advance! Nasko
creis@chromium.org changed reviewers: + boliu@chromium.org
Thanks for the detailed writeup in https://crbug.com/657720#c16! The fix seems reasonable from my perspective, and I like how the it fits into some similar checks. I can't think of virtual URLs in Chrome that would be invalid, so hopefully it's ok. The biggest risk that I can think of is Android Webview, where apps can specify unexpected virtual URLs using LoadDataWithBaseURL. Hopefully they can't use invalid URLs? boliu@, do you know? Otherwise, LGTM. https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:146: // malformed URLs. Clever! https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:196: EXPECT_EQ(GURL(), new_web_contents->GetLastCommittedURL()); Out of curiosity, does this return new_tab_url without your fix? Or something like http://bar.com/title2.html? (Maybe mention that in the comment.) https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:307: GURL virtual_url = entry.GetVirtualURL(); const ref? https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1246: if (!entry.get()) Is this stale? I don't see any changes to CreateNavigationEntry that would allow it to return null.
On 2016/10/26 23:07:09, Charlie Reis wrote: > Thanks for the detailed writeup in https://crbug.com/657720#c16! > > The fix seems reasonable from my perspective, and I like how the it fits into > some similar checks. I can't think of virtual URLs in Chrome that would be > invalid, so hopefully it's ok. > > The biggest risk that I can think of is Android Webview, where apps can specify > unexpected virtual URLs using LoadDataWithBaseURL. Hopefully they can't use > invalid URLs? boliu@, do you know? The honest answer is I don't know. I think general rule of thumb is if something happen to be allowed, then some app will happen to reply that behavior, intended or not. I found this bug that had the app pass in an invalid base url, and in that case, we just worked around the bug in webview: crbug.com/584924. We can probably do that same thing here if something goes wrong, so go for it and fingers crossed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:146: // malformed URLs. On 2016/10/26 23:07:09, Charlie Reis wrote: > Clever! Thanks :) https://codereview.chromium.org/2452443002/diff/20001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:196: EXPECT_EQ(GURL(), new_web_contents->GetLastCommittedURL()); On 2016/10/26 23:07:09, Charlie Reis wrote: > Out of curiosity, does this return new_tab_url without your fix? Or something > like http://bar.com/title2.html (Maybe mention that in the comment.) It returns the virtual URL set on the NavigationEntry, which isn't new_tab_url exactly. It has scheme prepended and one less : - "http://www.foo.com:/server-redirect?http%3A%2F%2Fbar.com%2Ftitle2.html". https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:307: GURL virtual_url = entry.GetVirtualURL(); On 2016/10/26 23:07:09, Charlie Reis wrote: > const ref? Done. https://codereview.chromium.org/2452443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1246: if (!entry.get()) On 2016/10/26 23:07:09, Charlie Reis wrote: > Is this stale? I don't see any changes to CreateNavigationEntry that would > allow it to return null. Oops, indeed. This is leftover from my previous try and I missed it.
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...
https://codereview.chromium.org/2452443002/diff/40001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2452443002/diff/40001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:199: // Note: In the current implementation, the URL is new_tab_url with a scheme nit: Maybe say "Before the bug was fixed" instead of "In the current implementation"? As soon as the comment lands, it won't be the current one anymore. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2452443002/diff/40001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2452443002/diff/40001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:199: // Note: In the current implementation, the URL is new_tab_url with a scheme On 2016/10/26 23:51:19, Charlie Reis wrote: > nit: Maybe say "Before the bug was fixed" instead of "In the current > implementation"? As soon as the comment lands, it won't be the current one > anymore. :) I meant to say that with the current implementation of the URL fixer, but I do like your suggestion better so used that.
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/2452443002/#ps60001 (title: "Fix comment.")
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 ========== Drop navigations to NavigationEntry with invalid virtual URLs. BUG=657720 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Drop navigations to NavigationEntry with invalid virtual URLs. BUG=657720 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Drop navigations to NavigationEntry with invalid virtual URLs. BUG=657720 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Drop navigations to NavigationEntry with invalid virtual URLs. BUG=657720 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e4ebe078840e65d673722e94f8251b334030b5e8 Cr-Commit-Position: refs/heads/master@{#428056} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e4ebe078840e65d673722e94f8251b334030b5e8 Cr-Commit-Position: refs/heads/master@{#428056} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
