New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 820070 link

Starred by 1 user
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Mar 2018
Cc:
█ bmcquade@chromium.org
█ nick@chromium.org
mattcary@chromium.org
alex...@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

GetSiteForUrl seems to no longer handle chrome:// urls in Debug

Project Member Reported by mattcary@chromium.org, Mar 8 2018 
I believe the changes at [1] to site_instance_impl.cc made in crrev.com/c/953129 breaks on chrome:// urls. The change appears to have made a unittest fail (see crrev.com/c/955625) when run on chrome://version. The cause seems to be that, as chrome://version is not a unique origin, we fall into the test case introduced in 953129. While this case should produce the same chrome: site, the new code calls origin.GetURL() which constructs a URL that fails in InitializeFromCanonicalSpec when run in debug mode.

I'm not sure the right way to untangle everything, for the time being I've disabled the unittest as that seems safer than reverting or otherwise mucking about with the more widely used site instance code.


[1] https://cs.chromium.org/chromium/src/content/browser/site_instance_impl.cc?rcl=b18148a55bfe0e2d8eba83d5cd58ba8105bbf9d7&l=437
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/479934c69aba6752d708b8221117924b66854193

commit 479934c69aba6752d708b8221117924b66854193
Author: Matthew Cary <mattcary@chromium.org>
Date: Thu Mar 08 15:51:01 2018

Disable DelayNavigationThrottleInstantiationTest.Instantiate.

It is breaking on only the chrome://version URL, but it's simpler to
disable the whole test rather than just one parameter.

Change-Id: I1341d503640058f18a914e6356de83ef8bb6fc1a
Bug:  820070 
TBR: bmcquade@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/955625
Reviewed-by: Matthew Cary <mattcary@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541804}
[modify] https://crrev.com/479934c69aba6752d708b8221117924b66854193/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc

Comment 2 by alex...@chromium.org, Mar 8 2018

Cc: alex...@chromium.org
Status: Assigned (was: Untriaged)
Do we actually need to call origin.GetURL().scheme() in the new case, as opposed to just origin.scheme()?

Comment 3 by lukasza@chromium.org, Mar 8 2018

Status: Started (was: Assigned)
alexmos@ - thanks for the suggestion.  Going via |origin.scheme()| instead of |origin.GetURL().scheme()| avoids the DCHECK and is probably something that we should have done from the beginining.  I'll put together a CL with this fix.

I've also opened a separate bug in case the root case of the DCHECK needs to be investigated -  issue 820194 .

Comment 4 by lukasza@chromium.org, Mar 8 2018

Cc: nick@chromium.org
WIP CL @ https://chromium-review.googlesource.com/956308
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d926a84917c4b9649c3cd5deb80dfd37279aef66

commit d926a84917c4b9649c3cd5deb80dfd37279aef66
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Mar 09 01:50:45 2018

Avoid unneeded call to Origin::GetURL from SiteInstance::GetSiteForURL.

Bug:  820070 
Change-Id: I61115868be9edf2bb6be1214e7eb4283faf1cf7b
Reviewed-on: https://chromium-review.googlesource.com/956308
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541986}
[modify] https://crrev.com/d926a84917c4b9649c3cd5deb80dfd37279aef66/content/browser/site_instance_impl.cc

Comment 6 by lukasza@chromium.org, Mar 9 2018

Status: Fixed (was: Started)
►

Sign in to add a comment