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

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

58 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Priority: -- → P2
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.
Attachment #8944518 - Flags: review?(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();

!!
Attachment #8944518 - Flags: review?(mcmanus) → review+
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07bbea084584
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.