1525025 - Resource timing tests introduced by wpt pr 14845 are unstable

Status: RESOLVED FIXED

(Core :: Performance, enhancement, P2)

Description

[James Graham [:jgraham]]

resource-timing/resource_connection_reuse_mixed_content_* tests are unstable on import and so need to be disabled.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

These tests are for performance API. I'm moving this to Performance module to ensure this is on their radar.

Comment 2

[Will Hawkins]

This should be solved thanks to the solution for 1539006 and 1525051.

If you can confirm, Mr. Graham, that'd be great. If you concur, we can close this!

Thanks,
Will

Comment 3

[James Graham [:jgraham]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1522621#c22

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:whawkins, maybe it's time to close this bug?

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:plawless, maybe it's time to close this bug?

Comment 6

[Sean Feng [:sefeng]]

The test is still buggy in Gecko and it's passing in Chromium and Safari, so very likely a bug in Gecko. I'll investigate it.

Removing the leave-open keyword since it's not needed.

Comment 7

[Sean Feng [:sefeng]]

The reason is connectStart (along with a few other timings) didn't return the same value as fetchStart when a persistent connection is used.

What should happen is when a persistent connection is used, connectStart is null, so we should return fetchStart.

However, It turns out the domainLookup timings were set https://searchfox.org/mozilla-central/rev/fa7f47027917a186fb2052dee104cd06c21dd76f/netwerk/protocol/http/nsHttpChannel.cpp#9125-9126, thus it's being returned which is not correct.

Valentin, Should we change connectStart.IsNull() check to !connectStart.IsNull()?

Comment 8

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

I agree that the condition is probably wrong.
I think the problem is that DNS prefetch is racy - for a persistent connection it could come back before we send the request, during, or after the response.
The reason for that check was so it keeps the order of the timestamps - meaning if we haven't set the request/connect timestamps yet (which should come after the DNS timestamps) then we set them. Otherwise the resource timing code will use the last non-null timestamp.

The problem here is that we don't know for sure if it's a persistent connection or not (yet).
What we could do is save those timestamps, and set them in OnStopRequest, but only if they are after fetchStart and before connectStart.

Comment 9

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

(In reply to Valentin Gosu [:valentin] (he/him) from comment #8)

What we could do is save those timestamps, and set them in OnStopRequest, but only if they are after fetchStart and before connectStart.

We already do that here, but I don't think that code gets called, because mDNSPrefetch gets nulled in OnLookupComplete.

Comment 10

[Sean Feng [:sefeng]]

Attachment: Bug 1525025 - Always update domainLookup time in OnStopRequest r=valentin

Currently the domainLookup time is updated in OnLookupComplete, this
is problematic because since the DNS prefetch is racy, we are unable
to tell if it's a persistent connection, and we don't want to update
domainLookup if it's a persistent connection.

This patch stops updading it in OnLookupComplete to only update
it in OnStopRequest.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac64bf5e69d3
Always update domainLookup time in OnStopRequest r=valentin,necko-reviewers

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ac64bf5e69d3