Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1911573002:
Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. (Closed)
|
Created:
4 years, 8 months ago by ncarter (slow) Modified:
4 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, ajwong+watch_chromium.org, site-isolation-reviews_chromium.org, Mike West, dmurph, brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTeach SiteInstance::GetSiteForURL() about blob and filesystem URLs.
Use url::Origin to do the heavy lifting.
This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general.
url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port.
BUG=602818, 564316, 490074, 605720
Committed: https://crrev.com/1dd47922979b128ce7ee8debf138b83c65661024
Cr-Commit-Position: refs/heads/master@{#390672}
Patch Set 1 #Patch Set 2 : More tests #Patch Set 3 : Mark two layout tests fixed #
Total comments: 2
Patch Set 4 : Attempt simple fix for NTP failures #
Total comments: 6
Patch Set 5 : scheme with port. #Patch Set 6 : Revert to patch set #4 #
Total comments: 6
Patch Set 7 : Add search unittest. #
Messages
Total messages: 31 (14 generated)
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. BUG=602818,564316 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. BUG=602818,564316,490074 ==========
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. BUG=602818,564316,490074 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. BUG=602818,564316,490074 ==========
nick@chromium.org changed reviewers: + creis@chromium.org
Charlie: PTAL https://codereview.chromium.org/1911573002/diff/40001/content/browser/site_in... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/1911573002/diff/40001/content/browser/site_in... content/browser/site_instance_impl_unittest.cc:321: EXPECT_EQ("[::1]", site_url.host()); These are added just for good measure, and pass before this CL. IP literals are important, since they exercise the case where GetDomainAndRegistry returns the empty string. https://codereview.chromium.org/1911573002/diff/40001/content/browser/site_in... content/browser/site_instance_impl_unittest.cc:373: EXPECT_EQ(GURL("http://google.com"), site_url); These 3 tests would have failed prior to this change.
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. BUG=602818,564316,490074 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This must be fixed by clearing the port. BUG=602818,564316,490074,605720 ==========
nick@chromium.org changed reviewers: + treib@chromium.org
Charlie: This is ready for your review now
creis@chromium.org changed reviewers: + samarth@chromium.org
[+samarth] Great. One thought about the chrome-search case below, since I'm leaning towards SCHEME_WITH_PORT. https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... chrome/browser/search/search.cc:496: replacements.ClearPort(); I'll defer to Samarth or someone familiar with the NTP code here, since this would be a problem if we later pulled the host out and tried to use it. It does seem a little counter-intuitive to me, since the only reason we need to do this is because the scheme was declared as not having a port. I'd say that we should declare the scheme as having a port instead, since it does have a network host embedded in it in some (if not all) cases. https://codereview.chromium.org/1911573002/diff/60001/content/browser/site_in... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/1911573002/diff/60001/content/browser/site_in... content/browser/site_instance_impl.cc:347: site += domain.empty() ? origin.host() : domain; nit: Maybe add a comment for which case this is for (e.g., IP addresses), since it's a little tricky to understand at first glance. (I agree that it's basically the same as what we did before.) https://codereview.chromium.org/1911573002/diff/60001/content/browser/site_in... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/1911573002/diff/60001/content/browser/site_in... content/browser/site_instance_impl_unittest.cc:313: test_url = GURL("http://2130706433/a.html"); This needs a comment. :) https://codereview.chromium.org/1911573002/diff/60001/content/browser/site_in... content/browser/site_instance_impl_unittest.cc:365: "blob:http://www.example.appspot.com:44/" Good case. Or you could use co.uk.
https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... chrome/browser/search/search.cc:496: replacements.ClearPort(); On 2016/04/22 18:11:09, Charlie Reis wrote: > I'll defer to Samarth or someone familiar with the NTP code here, since this > would be a problem if we later pulled the host out and tried to use it. It does > seem a little counter-intuitive to me, since the only reason we need to do this > is because the scheme was declared as not having a port. > > I'd say that we should declare the scheme as having a port instead, since it > does have a network host embedded in it in some (if not all) cases. We're erasing the underlying scheme anyway (which is usually https://, except in tests, where we allow it to be overridden) so even if we preserve the port, conversion from a chrome-search:// URL back to a network URL doesn't seem possible without extra information. I'm guessing that's why this extra erasure doesn't break tests -- they'd already be broken if they assumed https? Or to think about it differently: if we think of the chrome-search scheme as having a network port, is the default value 80 or 443?
LGTM https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... chrome/browser/search/search.cc:496: replacements.ClearPort(); On 2016/04/22 20:00:34, ncarter wrote: > On 2016/04/22 18:11:09, Charlie Reis wrote: > > I'll defer to Samarth or someone familiar with the NTP code here, since this > > would be a problem if we later pulled the host out and tried to use it. It > does > > seem a little counter-intuitive to me, since the only reason we need to do > this > > is because the scheme was declared as not having a port. > > > > I'd say that we should declare the scheme as having a port instead, since it > > does have a network host embedded in it in some (if not all) cases. > > We're erasing the underlying scheme anyway (which is usually https://, except in > tests, where we allow it to be overridden) so even if we preserve the port, > conversion from a chrome-search:// URL back to a network URL doesn't seem > possible without extra information. > > I'm guessing that's why this extra erasure doesn't break tests -- they'd already > be broken if they assumed https? > > Or to think about it differently: if we think of the chrome-search scheme as > having a network port, is the default value 80 or 443? That's a good point about losing the scheme. That does suggest that the port is unlikely to be needed from a chrome-search:// URL, since we can't meaningfully recover the original URL from this one. Thus, I think I'm ok with leaving it as SCHEME_WITHOUT_PORT and clearing it here. (Please add a comment about it, though, so we remember if we come back to it later.) The downside is that the chrome-search://foo.com URL can't distinguish between variations of foo.com if the default search provider changes (e.g., between ports or http/https), but that's of course a problem already today with http/https.
On 2016/04/22 20:20:46, Charlie Reis wrote: > LGTM > > https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... > File chrome/browser/search/search.cc (right): > > https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... > chrome/browser/search/search.cc:496: replacements.ClearPort(); > On 2016/04/22 20:00:34, ncarter wrote: > > On 2016/04/22 18:11:09, Charlie Reis wrote: > > > I'll defer to Samarth or someone familiar with the NTP code here, since this > > > would be a problem if we later pulled the host out and tried to use it. It > > does > > > seem a little counter-intuitive to me, since the only reason we need to do > > this > > > is because the scheme was declared as not having a port. > > > > > > I'd say that we should declare the scheme as having a port instead, since it > > > does have a network host embedded in it in some (if not all) cases. > > > > We're erasing the underlying scheme anyway (which is usually https://, except > in > > tests, where we allow it to be overridden) so even if we preserve the port, > > conversion from a chrome-search:// URL back to a network URL doesn't seem > > possible without extra information. > > > > I'm guessing that's why this extra erasure doesn't break tests -- they'd > already > > be broken if they assumed https? > > > > Or to think about it differently: if we think of the chrome-search scheme as > > having a network port, is the default value 80 or 443? > > That's a good point about losing the scheme. That does suggest that the port is > unlikely to be needed from a chrome-search:// URL, since we can't meaningfully > recover the original URL from this one. > > Thus, I think I'm ok with leaving it as SCHEME_WITHOUT_PORT and clearing it > here. (Please add a comment about it, though, so we remember if we come back to > it later.) > > The downside is that the http://chrome-search://foo.com URL can't distinguish between > variations of http://foo.com if the default search provider changes (e.g., between > ports or http/https), but that's of course a problem already today with > http/https. If these tests were being written from scratch today, we probably wouldn't need all this plumbing for test-only http (now that you can mock out cert verification with the embedded test server), and we could just require and assume https. Not that I'm volunteering to do that, but maybe that's an argument for SCHEME_WITH_PORT? It really makes no difference to me.
On 2016/04/22 20:30:10, ncarter wrote: > On 2016/04/22 20:20:46, Charlie Reis wrote: > > LGTM > > > > > https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... > > File chrome/browser/search/search.cc (right): > > > > > https://codereview.chromium.org/1911573002/diff/60001/chrome/browser/search/s... > > chrome/browser/search/search.cc:496: replacements.ClearPort(); > > On 2016/04/22 20:00:34, ncarter wrote: > > > On 2016/04/22 18:11:09, Charlie Reis wrote: > > > > I'll defer to Samarth or someone familiar with the NTP code here, since > this > > > > would be a problem if we later pulled the host out and tried to use it. > It > > > does > > > > seem a little counter-intuitive to me, since the only reason we need to do > > > this > > > > is because the scheme was declared as not having a port. > > > > > > > > I'd say that we should declare the scheme as having a port instead, since > it > > > > does have a network host embedded in it in some (if not all) cases. > > > > > > We're erasing the underlying scheme anyway (which is usually https://, > except > > in > > > tests, where we allow it to be overridden) so even if we preserve the port, > > > conversion from a chrome-search:// URL back to a network URL doesn't seem > > > possible without extra information. > > > > > > I'm guessing that's why this extra erasure doesn't break tests -- they'd > > already > > > be broken if they assumed https? > > > > > > Or to think about it differently: if we think of the chrome-search scheme as > > > having a network port, is the default value 80 or 443? > > > > That's a good point about losing the scheme. That does suggest that the port > is > > unlikely to be needed from a chrome-search:// URL, since we can't meaningfully > > recover the original URL from this one. > > > > Thus, I think I'm ok with leaving it as SCHEME_WITHOUT_PORT and clearing it > > here. (Please add a comment about it, though, so we remember if we come back > to > > it later.) > > > > The downside is that the http://chrome-search://foo.com URL can't distinguish > between > > variations of http://foo.com if the default search provider changes (e.g., > between > > ports or http/https), but that's of course a problem already today with > > http/https. > > If these tests were being written from scratch today, we probably wouldn't need > all this plumbing for test-only http (now that you can mock out cert > verification with the embedded test server), and we could just require and > assume https. > > Not that I'm volunteering to do that, but maybe that's an argument for > SCHEME_WITH_PORT? > > It really makes no difference to me. As I said on the bug, I think using SCHEME_WITH_PORT seems marginally better to me, but the current approach in the CL is likely to be fine as well.
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This must be fixed by clearing the port. BUG=602818,564316,490074,605720 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by changing the scheme to allow ports. BUG=602818,564316,490074,605720 ==========
nick@chromium.org changed reviewers: + brettw@chromium.org
I've switched to SCHEME_WITH_PORT +brettw for chrome_content_client.cc
On 2016/04/27 22:07:27, ncarter wrote: > I've switched to SCHEME_WITH_PORT > > +brettw for chrome_content_client.cc I'm having a little trouble figuring out the reason for the HOST_WITH_PORT change which means other people looking at this code will to. The CL description should talk about why we're changing this. And maybe the code should say why. It's not clear why this one needs a port while the other ones int he list don't.
On 2016/04/27 22:26:22, brettw wrote:
> On 2016/04/27 22:07:27, ncarter wrote:
> > I've switched to SCHEME_WITH_PORT
> >
> > +brettw for chrome_content_client.cc
>
> I'm having a little trouble figuring out the reason for the HOST_WITH_PORT
> change which means other people looking at this code will to. The CL
description
> should talk about why we're changing this. And maybe the code should say why.
> It's not clear why this one needs a port while the other ones int he list
don't.
The switch to SCHEME_WITH_PORT was the change between PS4 and PS5, and the try
results suggests that it doesn't work after all. It's likely I'll go back to the
PS4 approach of clearing the port, but if not, I'll use the following comment:
// The chrome-search:// scheme is used with hosts that correspond to the
// network host of the search provider. These can have ports, so this scheme
// allows ports.
{chrome::kChromeSearchScheme, url::SCHEME_WITH_PORT},
The linked bug, http://crbug.com/605720, has more discussion.
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by changing the scheme to allow ports. BUG=602818,564316,490074,605720 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port. BUG=602818,564316,490074,605720 ==========
nick@chromium.org changed reviewers: - brettw@chromium.org
I've switched back to clearing the port, since SCHEME_WITH_PORT causes other problems: https://bugs.chromium.org/p/chromium/issues/detail?id=605720#c8 Removing brettw from reviewer list. samarth, I need your approval.
A few comments but this lgtm for search. https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:493: // Replace the scheme with "chrome-search:". Can you extend this comment to explain why the port is also removed? https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:496: replacements.ClearPort(); nit: this line seems out of order since the search_scheme defined above is used below this line. Move it above L495? https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:496: replacements.ClearPort(); Can you add a test for this in search_unittest.cc?
Done. PTAL at the unittest. https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:493: // Replace the scheme with "chrome-search:". On 2016/04/28 20:25:03, samarth wrote: > Can you extend this comment to explain why the port is also removed? Done. https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:496: replacements.ClearPort(); On 2016/04/28 20:25:03, samarth wrote: > nit: this line seems out of order since the search_scheme defined above is used > below this line. Move it above L495? Done. https://codereview.chromium.org/1911573002/diff/100001/chrome/browser/search/... chrome/browser/search/search.cc:496: replacements.ClearPort(); On 2016/04/28 20:25:03, samarth wrote: > Can you add a test for this in search_unittest.cc? Done.
lgtm
The CQ bit was checked by nick@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/1911573002/#ps120001 (title: "Add search unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911573002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911573002/120001
Message was sent while issue was closed.
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port. BUG=602818,564316,490074,605720 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port. BUG=602818,564316,490074,605720 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1dd47922979b128ce7ee8debf138b83c65661024 Cr-Commit-Position: refs/heads/master@{#390672}
Message was sent while issue was closed.
Description was changed from ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port. BUG=602818,564316,490074,605720 ========== to ========== Teach SiteInstance::GetSiteForURL() about blob and filesystem URLs. Use url::Origin to do the heavy lifting. This fixes FrameTreeBrowsertest.NavigateGrandchildToBlob under --site-per-process, and fixes process transfers for blob URLs in general. url::Origin extraction exposed a bug where tests encountered chrome-search:// URLs with ports. This is fixed by clearing the port. BUG=602818,564316,490074,605720 Committed: https://crrev.com/1dd47922979b128ce7ee8debf138b83c65661024 Cr-Commit-Position: refs/heads/master@{#390672} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
