Last Comment Bug 675615 - Fix test for 570341 to support keep-alive connections
: Fix test for 570341 to support keep-alive connections
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla8
Assigned To: Igor Bazarny
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 469228
  Show dependency treegraph
 
Reported: 2011-08-01 07:10 PDT by Honza Bambas (:mayhemer)
Modified: 2011-08-09 08:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.64 KB, patch)
2011-08-01 07:10 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
Fix to navigation timing implementation for keep-alive connections (4.87 KB, patch)
2011-08-04 08:54 PDT, Igor Bazarny
bugs: review+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-08-01 07:10:26 PDT
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.
Comment 1 Honza Bambas (:mayhemer) 2011-08-01 07:28:09 PDT
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.
Comment 2 Igor Bazarny 2011-08-03 08:40:29 PDT
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
Comment 3 Honza Bambas (:mayhemer) 2011-08-03 08:46:44 PDT
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.
Comment 4 Igor Bazarny 2011-08-03 08:56:02 PDT
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.
Comment 5 Honza Bambas (:mayhemer) 2011-08-03 09:08:08 PDT
(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.
Comment 6 Igor Bazarny 2011-08-04 08:54:44 PDT
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.
Comment 7 Honza Bambas (:mayhemer) 2011-08-04 11:56:07 PDT
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.
Comment 8 Olli Pettay [:smaug] 2011-08-05 07:25:15 PDT
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.
Comment 9 Honza Bambas (:mayhemer) 2011-08-05 08:34:05 PDT
Confirming this works with keep-alive support on httpd.js.  Thanks.
Comment 10 Igor Bazarny 2011-08-05 10:13:57 PDT
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.
Comment 11 Olli Pettay [:smaug] 2011-08-05 10:21:17 PDT
Honza, since you've been testing the patch, could you push it to try, 
and if everything looks ok, to m-c also?
Comment 12 Honza Bambas (:mayhemer) 2011-08-05 12:41:19 PDT
I'll try both, but might not happen sooner then on Monday.
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2011-08-05 18:55:05 PDT
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?)

Note You need to log in before you can comment on or make changes to this bug.