1933250 - Extend destination/initiatorType tests to cover most values from the spec

Status: NEW

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Julian Descottes [:jdescottes]]

At the moment, nsITimedChannel initiatorType can sometimes have unexpected values.

For instance, for a navigation request, the HTML spec says:

If navigable's container is non-null:

• [...]
• Set request's destination to navigable's container's local name.
• If sourceSnapshotParams's fetch client is navigable's container document's relevant settings object, then set request's initiator type to navigable's container's local name.

I think in case of a top level navigation, navigable's container is null? (from documents loaded by an iframe, it's set to the iframe element, but for a top level navigable it shouldn't be anything).

Furthermore, the spec says:

This ensure that only container-initiated navigations are reported to resource timing.

So I imagine that for all top level requests, the destination/initiatorType values must be filtered out at some point, since they shouldn't actually be exposed to the performance resource timing API.

If we follow the spec to the letter, it means that destination should be empty string and initiatorType should be null for top level loads?

In any case at the moment, the initiatorType we get for such requests is "browser" which is not listed in the fetch spec, and the destination is "document" (which is in the spec, but based on the analysis above, I think we should just have empty string) ?

Comment 1

[Julian Descottes [:jdescottes]]

Valentin, do you know about other potential discrepancies between nsITimedChannel.initiatorType and fetch's initiatorType?
If it's only about top level loads, I might just special case those at the moment, but to be fair it's a bit hard to track down all the cases in the spec which can lead to have a specific destination/initiatorType set in the fetch spec.

Maybe there is a specific spot on the platform side where we translate our internal initiatorType to something fully spec compliant?

Comment 2

[Julian Descottes [:jdescottes]]

Forgot to send the ni? Valentin: see above, also additional question:

• do you have an idea about what kind of requests have an empty destination/initiatorType? is it something we can use to flag navigation requests if we apply what I mentioned in the comment above?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #2)

Forgot to send the ni? Valentin: see above, also additional question:

• do you have an idea about what kind of requests have an empty destination/initiatorType? is it something we can use to flag navigation requests if we apply what I mentioned in the comment above?

I think nsITimedChannel.initiatorType should match the fetch/resourceTiming definition exactly.
If there's a difference, we probably need to update our code.

initiatorType is currently set to "browser" via nsDocShell::CreateAndConfigureRealChannelForLoadState.
I'm not aware of any other differences with initiatorType. I think if a callsite creates a channel that doesn't set the initiatorType, we might also get an empty initiatorType - but I'm not sure if that still ever happens.

Maybe there is a specific spot on the platform side where we translate our internal initiatorType to something fully spec compliant?

I don't think we do. The initiatorType saved on the channel is mirrored exactly in resource/navigation timing except for this

Comment 4

[Julian Descottes [:jdescottes]]

Sounds good, so it looks like toplevel loads & "browser" should be the only exception.

I can add a check to make sure the values we emit are only the ones in the fetch spec, probably in our tests (unless we have a shared list of fetch initiatorTypes in the codebase already)

Comment 5

[Julian Descottes [:jdescottes]]

Based on the conversation, moving to task. For now we will try to be more exhaustive in our BiDi test to make sure we don't expose strings not expected in the spec.

Thanks for the feedback!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Julian, are we aware already of other entries that might not be compliant with the spec? If not we most likely have to wait for feedback from users and then react on it. As such I would drop it for now from the current milestone.

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #6)

Julian, are we aware already of other entries that might not be compliant with the spec? If not we most likely have to wait for feedback from users and then react on it. As such I would drop it for now from the current milestone.

I don't know in details, but we can expect failures for all wpt initiatorType tests currently failing for Firefox:
https://searchfox.org/mozilla-central/search?q=The+initiator+type+for&path=ini&case=false&regexp=false

so 12 scenarios at least.