New issue
Advanced search Search tips

Issue 922365 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[Resource-Timing] `name` provides the post-redirection URL for http=>https redirections

Project Member Reported by yoavweiss@chromium.org, Jan 16 (6 days ago)

Issue description

Chrome Version: M73
OS: N/A

What steps will reproduce the problem?
(1) Run the test on https://chromium-review.googlesource.com/c/chromium/src/+/1411929
(2) Cry
(3) Cry some more

What is the expected result?
Test should pass


What happens instead?
It fails



Please use labels and text to provide additional information.

If this is a regression (i.e., worked before), please consider using
the bisect tool (https://www.chromium.org/developers/bisect-builds-py)
to help us identify the root cause and more rapidly triage the issue.

For graphics-related bugs, please copy/paste the contents of the
about://gpu page at the end of this report.


 

Comment 1 by yoavweiss@chromium.org, Jan 16 (6 days ago)

Cc: yhirano@chromium.org
My analysis so far:
For cross-origin sync XHR (at least), redirections happen in ThreadableLoader, which overrides the original ResourceRequest when it calls[1] LoadRequest. I couldn't find a way to get a hold of the original ResourceRequest at GenerateResourceTiming [2].

yhirano@ - Is ThreadableLoader doing that for other types of XHR/Fetch as well? (just to understand how common we should expect the problem to be)
Is there an easy way to avoid doing those redirections in a way that overrides the origin ResourceRequest?

One way I can think of to solve this is to keep a copy of the old ResourceRequest (or at least its URL) and consult that for the name property. Does that make sense?

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/threadable_loader.cc?type=cs&g=0&l=463
[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/timing/performance.cc?q=GenerateResourceTiming&sq=package:chromium&g=0&l=487

Comment 2 by yhirano@chromium.org, Jan 16 (6 days ago)

I think so.

If this issue is not urgent, out-of-blink CORS will fix it.

Comment 3 by yoavweiss@chromium.org, Jan 16 (6 days ago)

When is that supposed to land?

Comment 4 by yhirano@chromium.org, Jan 16 (6 days ago)

We're running finch on Canary. See issue 736308.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit 5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad
Author: Yoav Weiss <yoavweiss@chromium.org>
Date: Wed Jan 16 18:06:39 2019

[Resource-Timing] `name` is wrong for http to https redirection

According to the spec, the `name` attribute MUST provide the
pre-redirection URL. In the case of http=>https redirections,
that's not the case and the post-redirection URL is provided.
This CL fixes that.

BUG= 922365 

Change-Id: Ic9acf354ef66d654ab41259aa0af297a56e775ce
Reviewed-on: https://chromium-review.googlesource.com/c/1411929
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623293}
[modify] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/renderer/core/loader/threadable_loader.cc
[modify] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/renderer/platform/loader/fetch/resource_request.cc
[modify] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/renderer/platform/loader/fetch/resource_request.h
[rename] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/web_tests/external/wpt/resource-timing/redirects.sub.html
[add] https://crrev.com/5ba90c2f9ee6e23c0ff0b22bc4ef0ae9477c57ad/third_party/blink/web_tests/external/wpt/resource-timing/resources/blank_page_green.htm.headers

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 17 (5 days ago)

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

commit 9dc341fd7d70ef121bec311a1b7eaa1dce5828c0
Author: Yoav Weiss <yoavweiss@chromium.org>
Date: Thu Jan 17 11:04:47 2019

Renamed ResourceRequest::OriginalUrl to InitialUrlForResourceTiming

Following feedback on [1], this is renaming OriginalUrl and commenting
it, to make it clear that it's a temporary stop-gap that should not be
used beyond the scope of ResourceTiming and should be removed once
Out-of-Blink CORS is shipped.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1411929

Bug:  922365 
Change-Id: I0bac3a13a4e765270cb911eaa1ab728bdd369d90
Reviewed-on: https://chromium-review.googlesource.com/c/1415261
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623641}
[modify] https://crrev.com/9dc341fd7d70ef121bec311a1b7eaa1dce5828c0/third_party/blink/renderer/core/loader/threadable_loader.cc
[modify] https://crrev.com/9dc341fd7d70ef121bec311a1b7eaa1dce5828c0/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/9dc341fd7d70ef121bec311a1b7eaa1dce5828c0/third_party/blink/renderer/platform/loader/fetch/resource_request.cc
[modify] https://crrev.com/9dc341fd7d70ef121bec311a1b7eaa1dce5828c0/third_party/blink/renderer/platform/loader/fetch/resource_request.h

Comment 7 by yoavweiss@chromium.org, Jan 17 (5 days ago)

Status: Fixed (was: Assigned)

Sign in to add a comment