Fix test for 570341 to support keep-alive connections

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mayhemer, Assigned: Igor Bazarny)

Tracking

(Blocks: 1 bug)

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 549778 [details] [diff] [review]
v1

As part of the work to make httpd.js support keep-alive connections test docshell/test/test_bug570341.html needs a little tweak.

When the iframe is loaded using a keep-alive connection, some events (dnsLookup and connection) are not stamped, because they simply don't occur.

Solution is to force cache bypass reload of the iframe.  Such force reload closes all keep-alive connections with the host.
Attachment #549778 - Flags: review?(igor.bazarny)
(Reporter)

Comment 1

6 years ago
Other (probably cleaner) solution is to publish API for closing connections per host.  We may do that in a followup as it is not that trivial as this solution.
(Assignee)

Comment 2

6 years ago
Does test fail for persistent connection? If some events don't occur, there should be a reasonable default for them, as described in the spec, most likely fetchStart like for domainLookupStart, see https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#nt-domain-lookup-start
Test shouldn't break in this case.
We probably need tests for both cases if there is a way to enforce persistent connection
(Reporter)

Comment 3

6 years ago
Yes, we could simply run the test twice: ones as it is in the unfixed version and ones with the hard reload (what is my fix).

I will go through the spec, but problem is that fixing the code according the spec might take much longer time then having the test server support keep-alive.  And it would block more important things to finish.
(Assignee)

Comment 4

6 years ago
I'd expect that implementation is correct and should use fetchStart for persistent connection, so not much changes are necessary.
Do we need to fix tests because of breakage or because we need to cover the case with new connection?
If tests break, I actually need to fix the API code, not hide the issue by enforcing a new connection in tests.
(Reporter)

Comment 5

6 years ago
(In reply to comment #4)
> I'd expect that implementation is correct and should use fetchStart for
> persistent connection, so not much changes are necessary.

Aha, so the test just needs some extending, do I understand correctly?

> Do we need to fix tests because of breakage or because we need to cover the
> case with new connection?

I think the letter.  Mochitest testing server is by now always closing a connection after a request has been satisfied.  So, if the test is not already prepared to cover also keep-alive connections, then it must be extended to do it.

> If tests break, I actually need to fix the API code, not hide the issue by
> enforcing a new connection in tests.

You seem to know more then me about this.  Feel free to obsolete my patch and make yours.  My patch just enforces a new connection to let the test run on it.

Patches to make the tests server use keep-alive connections are in bug 469228 (there is also a single merged patch to apply), then build network/test/httpserver and testing/mochitest dirs.
(Assignee)

Comment 6

6 years ago
Created attachment 550708 [details] [diff] [review]
Fix to navigation timing implementation for keep-alive connections

Fix to the navigation.timing implementation. It turns out I was not implement fallback to fetchStart for missing channel-level events.

Bugzilla doesn't let me obsolete the previous attachment which seems unnecessary now--with new implementation test passes.
Attachment #550708 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 7

6 years ago
Comment on attachment 549778 [details] [diff] [review]
v1

I'll just drop the review request for now.  Let's see how Igor's patch goes and obsolete after it gets in.
Attachment #549778 - Flags: review?(igor.bazarny)

Comment 8

6 years ago
Comment on attachment 550708 [details] [diff] [review]
Fix to navigation timing implementation for keep-alive connections

Yeah, Fetch start is probably the best value we can get here.
Attachment #550708 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 9

6 years ago
Confirming this works with keep-alive support on httpd.js.  Thanks.
(Assignee)

Comment 10

6 years ago
Actually, fetchStart is what the spec says, and it was in the code before some of refactorings.

Thanks for the review, I also need help with running the patch trough try server and submitting it.
Honza, since you've been testing the patch, could you push it to try, 
and if everything looks ok, to m-c also?
(Reporter)

Comment 12

6 years ago
I'll try both, but might not happen sooner then on Monday.
Pushed to try: http://tbpl.mozilla.org/?tree=Try&pusher=cbiesinger@gmail.com&rev=53b8006946b0

We'll want this on aurora, right? (did webtiming end up on beta yet?)
(Reporter)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/820ac76cec76
Assignee: nobody → igor.bazarny
Status: NEW → ASSIGNED
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/820ac76cec76
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.