Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should redirected navigations have a workerStart value if a previous redirect URL was intercepted #118

Closed
wanderview opened this issue Oct 5, 2017 · 7 comments · Fixed by #217

Comments

@wanderview
Copy link
Member

wanderview commented Oct 5, 2017

Consider the following situation:

  1. Use opens tab a.com
  2. a.com is intercepted by a service worker which results in a redirect to b.com
  3. b.com is loaded

Two questions:

a. Should there be a .workerStart for the a.com interception? There would if this was a subresource request.
b. Should the .workerStart value be controlled by the Timing-Allow-Origin header? The redirectStart/redirectEnd values are controlled by that header, but workerStart doesn't mention it.

If the answer to (a) is "yes", then consider:

  1. a.com is intercepted by a SW and redirects to b.com
  2. b.com is intercepted by a SW and redirects to c.com
  3. c.com is loaded

Should the workerStart for a.com or b.com be shown?

@igrigorik
Copy link
Member

Should the .workerStart value be controlled by the Timing-Allow-Origin header?

Yes. Good catch, we should update processing (step 8) to reset it to 0 if timing allow check fails.

Should the workerStart for a.com or b.com be shown?

If TAO check passes we show timestamps for the "final resource in redirection", and 0's otherwise. So, in the example above it would be timestamps for c.com and workerStart would capture worker startup time for c.com (if one is present). This should be consistent with: https://www.w3.org/TR/resource-timing-2/#dom-performanceresourcetiming-workerstart

Does that make sense? /cc @toddreifsteck

@wanderview
Copy link
Member Author

If TAO check passes we show timestamps for the "final resource in redirection", and 0's otherwise. So, in the example above it would be timestamps for c.com and workerStart would capture worker startup time for c.com (if one is present).

Ok, thats not what the current WPT test verifies:

https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/resource-timing.https.html#L21

And I guess its not what chrome does since it passes this test:

https://wpt.fyi/service-workers/service-worker/resource-timing.https.html

@yoavweiss
Copy link
Contributor

OK, so if I'm reading this correctly, in order to resolve this we need to:

  1. Clarify that workerStart is gated on TAO and add tests to that effect
  2. Add spec language that makes sure workerStart is applied to the final resource in the redirection chain
    2a) If this resource has a SW, workerStart should apply to it.
    2b) If it doesn't, workerStart should be 0, even if previous resources had a SW.

(1) seems like a no brainer, so might be worthwhile to spin it off.
OTOH, I'm not sure (2b) is the desired behavior as far as the use-cases go, as it can render the attribute less useful, although I don't know how often such a scenario happens.

@igrigorik - I remember a plan to better expose full redirect chains, but can't find an issue tracking it. Am I remembering correctly?

@igrigorik
Copy link
Member

@yoavweiss you're looking for #21.

@yoavweiss
Copy link
Contributor

Following discussion on the last call, I believe both issues here (TAO & last redirected SW) are only impactful/observable in the NavigationTiming case. I'll open an issue there and close this one.

@yoavweiss
Copy link
Contributor

Opened a couple of issues on NT, so closing this one

@yoavweiss yoavweiss reopened this Nov 19, 2019
@yoavweiss
Copy link
Contributor

Reopening as we missed a case where this does impact RT - iframe RT entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants