1430738 - Investigate why secureConnectionStart is incorrect in some cases for https://github.com/w3c/web-platform-tests/pull/9040

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1430738.patch

If a halfOpenSocket is created with a nsHttpTransaction, not a NullTransaction, all resolveing_host, resolved_host , connecting timing info is store in the transaction. The bug is that we always reset its timing info when setup nsHttpConnection, because nsHttpconnection::mBootstrappedTimings is only set if halfOpenSocket is created with a NullHttpTransaction:
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnectionMgr.cpp#4771

The bug that this test produces is that we clear the nsHttpTransaction timing but than we get TFO triggered tls start. And after that nsHttpChannel::OnLookupComplete gets call that sets DNS timing(from a DNSprefetch). The DNS timing is set only if connectionStart  is null and in this case it is because the nsHttpTransaction timing has been cleared. The DNS timing from the prefetch are newer than the one used bug the HalfOpenSocket.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8944518 [details] [diff] [review]
bug_1430738.patch

Review of attachment 8944518 [details] [diff] [review]:
-----------------------------------------------------------------

thx

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +2535,5 @@
>    case NS_NET_STATUS_RESOLVING_HOST:
>      mBootstrappedTimings.domainLookupStart = TimeStamp::Now();
>      break;
>    case NS_NET_STATUS_RESOLVED_HOST:
> +    mBootstrappedTimings.domainLookupEnd = TimeStamp::Now();

!!

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> Comment on attachment 8944518 [details] [diff] [review]
> bug_1430738.patch
> 
> Review of attachment 8944518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thx
> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +2535,5 @@
> >    case NS_NET_STATUS_RESOLVING_HOST:
> >      mBootstrappedTimings.domainLookupStart = TimeStamp::Now();
> >      break;
> >    case NS_NET_STATUS_RESOLVED_HOST:
> > +    mBootstrappedTimings.domainLookupEnd = TimeStamp::Now();
> 
> !!

While debugging this I notice that there is a bug here.  NS_NET_STATUS_RESOLVED_HOST is signal that a lookup ended not started. NS_NET_STATUS_RESOLVING_HOST is a lookup starting.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07bbea084584
Fix the channel timing info if a halfOpenSocket is opened with a non-null httpTransaction. r=mcmanus

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/07bbea084584

Comment 6

[npm]

Is this really fixed? The WPT bot is still saying that the test fails on Firefox nightly, and my last attempt was Jan 30 (which I expect would already include the changes referenced by this bug):
https://travis-ci.org/w3c/web-platform-tests/jobs/335389026