Closed Bug 772589 Opened 8 years ago Closed 3 years ago

Implement the secureConnectionStart property for the PerformanceTiming interface

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan, Assigned: mcmanus)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

This new property should use the new DOM bindings (see the patch in bug 749101).
Note to whoever implements this: the following WebIDL property needs to be uncommented when you fix this bug, and the comment in this commit needs to be removed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb282ed64bf7
This is the only part of the Navigation Timing test suite we fail...
Attachment #8885914 - Flags: review?(tnguyen)
Attachment #8885914 - Flags: review?(dd.mozilla)
Attachment #8885914 - Flags: review?(bkelly)
To my kind reviewers - you might want to redirect the r? .. I'll understand.

I figure Ben for dom, Tim for devtools, and dragana for necko.

dragana, there are quite a few necko pieces in here.. the most interesting does a better job of associating null-transaction timings with actual transactions.. which gives us more data..

ben, I think the dom stuff is pretty straight forward. we pass more wp tests. The most interesting part is probably dealing with the null-transaction (typically sepculative) information which creates timestamps earlier than the time of fetch start.

Tim, I'm pretty unfamiliar with devtools.. I did my best, just let me know what else I can do. dev-tools seems to rely heavily on channel based events which, as described in the last two paragraphs, don't always hit the right channel if the dns or connection is speculative before the channel (as it doesn't exist yet). I added some code to check the nsITimedChannel information in that case to get more data into the dev tools that we've been having.
Attachment #8885914 - Flags: review?(tnguyen) → review?(ntim.bugs)
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162044

The devtools changes look fine to me, but maybe Honza will have comments to make.
Attachment #8885914 - Flags: review?(ntim.bugs)
Attachment #8885914 - Flags: review?(odvarko)
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162058

::: netwerk/protocol/http/NullHttpChannel.cpp:699
(Diff revision 1)
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +NullHttpChannel::GetSecureConnectionStart(mozilla::TimeStamp *aConnectEnd)
> +{

nit: rename the parameter. it is not a connect end.

::: netwerk/protocol/http/NullHttpTransaction.h:61
(Diff revision 1)
>    // NullHttpTransaction will be activated on the connection immediately after
>    // creation and be never put in a pending queue, so it's OK to just return 0.
>    uint64_t TopLevelOuterContentWindowId() override { return 0; }
>  
> +  TimingStruct Timings() { return mTimings; }
> +  

nit: whitespace

::: netwerk/protocol/http/nsHttpConnection.cpp:548
(Diff revision 1)
> +    if (mBootstrappedTimings.secureConnectionStart.IsNull() &&
> +        !mBootstrappedTimings.connectEnd.IsNull()) {
> +        mBootstrappedTimings.secureConnectionStart = mBootstrappedTimings.connectEnd;
> +        mBootstrappedTimings.connectEnd = TimeStamp::Now();
> +    }
> +            

nit white space

::: netwerk/protocol/http/nsHttpConnection.cpp:600
(Diff revision 1)
> +        if (hTrans) {
> +            hTrans->BootstrapTimings(mBootstrappedTimings);
> +        }
> +        mBootstrappedTimings = TimingStruct();
> +    }
> +    

white space

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:4345
(Diff revision 1)
>  
> +    NullHttpTransaction *nullTrans = mTransaction->QueryNullTransaction();
> +    if (nullTrans) {
> +        conn->BootstrapTimings(nullTrans->Timings());
> +    }
> +    

nit: white space

::: netwerk/protocol/http/nsHttpTransaction.h:164
(Diff revision 1)
>  
>      mozilla::TimeStamp GetDomainLookupStart();
>      mozilla::TimeStamp GetDomainLookupEnd();
>      mozilla::TimeStamp GetConnectStart();
> +    mozilla::TimeStamp GetSecureConnectionStart();
> +    

white space

::: toolkit/components/telemetry/Histograms.json:1903
(Diff revision 1)
>      "high": 30000,
>      "n_buckets": 50,
>      "description": "HTTP page channel: DNS lookup time (ms)"
>    },
> +  "HTTP_PAGE_TLS_HANDSHAKE": {
> +    "record_in_processes": ["main", "content"],

This is not used yet?

::: toolkit/components/telemetry/Histograms.json:2049
(Diff revision 1)
>      "high": 30000,
>      "n_buckets": 50,
>      "description": "HTTP subitem channel: DNS lookup time (ms)"
>    },
> +  "HTTP_SUB_TLS_HANDSHAKE": {
> +    "record_in_processes": ["main", "content"],

and this?
Attachment #8885914 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162074

r=me with comments addressed.  I reviewed `dom/*` and WPT tests.  Thanks.

::: dom/performance/PerformanceTiming.cpp:93
(Diff revision 1)
> +
> +    // the performance timing api essentially requires that the event timestamps
> +    // are >= asyncOpen().. but in truth the browser engages in a number of
> +    // speculative activities that sometimes mean connections and lookups begin
> +    // earlier. Workaround that here by just using asyncOpen as the minimum
> +    // timestamp for dns and connection info.

Is there a spec issue filed on this?  If not please file one.  Also include a link to the issue here.

::: dom/webidl/PerformanceTiming.webidl:23
(Diff revision 1)
>    readonly attribute unsigned long long redirectEnd;
>    readonly attribute unsigned long long fetchStart;
>    readonly attribute unsigned long long domainLookupStart;
>    readonly attribute unsigned long long domainLookupEnd;
>    readonly attribute unsigned long long connectStart;
> +  readonly attribute unsigned long long secureConnectionStart;

Why did you re-order this?  The spec webidl has `secureConnectionStart` between `connectEnd` and `requestStart`.

Please use the same ordering as the spec to make it easier to compare the webidl file in the future.
Attachment #8885914 - Flags: review?(bkelly) → review+
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162058

> This is not used yet?

that's the code in nsLoadGroup that uses the weird macro.

> and this?

also the code from nsloadgroup with the weird macro
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162074

> Is there a spec issue filed on this?  If not please file one.  Also include a link to the issue here.

can you send me a link to the right repo for an issue? Or just file one with me cc'd and I'll chip in..

> Why did you re-order this?  The spec webidl has `secureConnectionStart` between `connectEnd` and `requestStart`.
> 
> Please use the same ordering as the spec to make it easier to compare the webidl file in the future.

weird; I put it that way because temporally that's the order they happen in. I wonder why the spec doesn't do that. I'll put it back.
(In reply to Patrick McManus [:mcmanus] from comment #10)
> Comment on attachment 8885914 [details]
> Bug 772589 - Implement the secureConnectionStart property for the
> PerformanceTiming interface
> 
> https://reviewboard.mozilla.org/r/156694/#review162074
> 
> > Is there a spec issue filed on this?  If not please file one.  Also include a link to the issue here.

I'll leave it to you to file since you understand the issue better, if thats ok.  I think this is the place:

https://github.com/w3c/navigation-timing/issues
(In reply to Patrick McManus [:mcmanus] from comment #9)
> Comment on attachment 8885914 [details]
> Bug 772589 - Implement the secureConnectionStart property for the
> PerformanceTiming interface
> 
> https://reviewboard.mozilla.org/r/156694/#review162058
> 
> > This is not used yet?
> 
> that's the code in nsLoadGroup that uses the weird macro.
> 
> > and this?
> 
> also the code from nsloadgroup with the weird macro

Yes, sorry...
on rereading the editor's draft I think the code is ok. It really only talks about persistent connections, of which this is a cousin.. and in those scenarios it recommends making the early values be fetchStart - which is what the patch does.

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #11)
> (In reply to Patrick McManus [:mcmanus] from comment #10)
> > Comment on attachment 8885914 [details]
> > Bug 772589 - Implement the secureConnectionStart property for the
> > PerformanceTiming interface
> > 
> > https://reviewboard.mozilla.org/r/156694/#review162074
> > 
> > > Is there a spec issue filed on this?  If not please file one.  Also include a link to the issue here.
> 
> I'll leave it to you to file since you understand the issue better, if thats
> ok.  I think this is the place:
> 
> https://github.com/w3c/navigation-timing/issues
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review162872

Looks reasonable to me.

R+ assuming Try is green.

Just one note related to HAR spec.
http://www.softwareishard.com/blog/har-12-spec/#timings

Honza

::: devtools/shared/webconsole/network-monitor.js:1396
(Diff revision 2)
>        return {
>          total: 0,
>          timings: {
>            blocked: 0,
>            dns: 0,
> +          tls: 0,

A note: HAR Spec doesn't have 'tls' field. I think it should go into `ssl` (or a custom `_tls`).

Honza
Attachment #8885914 - Flags: review?(odvarko) → review+
Attachment #8885914 - Flags: review?(francois)
:francois - I just r'd you becuase there are 2 new telemetry entries.. the set of webperformance timings is already being submitted to telemetry and this patch just adds a new field to that structure - the tls handshake time is separated from the overall connection time.
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review163704

datareview+ with comments below

::: toolkit/components/telemetry/Histograms.json:1905
(Diff revision 3)
>      "description": "HTTP page channel: DNS lookup time (ms)"
>    },
> +  "HTTP_PAGE_TLS_HANDSHAKE": {
> +    "record_in_processes": ["main", "content"],
> +    "expires_in_version": "never",
> +    "alert_emails": ["necko@mozilla.com"],

We're now asking for a person's email address to be added here. Having a team in there is fine, but we also want an individual who is responsible for it.

::: toolkit/components/telemetry/Histograms.json:1918
(Diff revision 3)
>      "record_in_processes": ["main", "content"],
>      "expires_in_version": "never",
>      "kind": "exponential",
>      "high": 30000,
>      "n_buckets": 50,
> -    "description": "HTTP page channel: TCP connection setup (ms)"
> +    "description": "HTTP page channel: TCP SYN to Ready for HTTP (ms)"

It sounds like you're changing what this probe is measuring.

Is it worth creating a new `HTTP_PAGE_TCP_CONNECTION_2` probe to avoid mixing data from the two different measurements?

::: toolkit/components/telemetry/Histograms.json:2051
(Diff revision 3)
>      "description": "HTTP subitem channel: DNS lookup time (ms)"
>    },
> +  "HTTP_SUB_TLS_HANDSHAKE": {
> +    "record_in_processes": ["main", "content"],
> +    "expires_in_version": "never",
> +    "alert_emails": ["necko@mozilla.com"],

Please add a person's email address here too.

::: toolkit/components/telemetry/Histograms.json:2064
(Diff revision 3)
>      "record_in_processes": ["main", "content"],
>      "expires_in_version": "never",
>      "kind": "exponential",
>      "high": 30000,
>      "n_buckets": 50,
> -    "description": "HTTP subitem channel: TCP connection setup (ms)"
> +    "description": "HTTP subitem channel: TCP SYN to Ready for HTTP (ms)"

Same comment as for `HTTP_PAGE_TCP_CONNECTION`.
Attachment #8885914 - Flags: review?(francois) → review+
Comment on attachment 8885914 [details]
Bug 772589 - Implement the secureConnectionStart property for the PerformanceTiming interface

https://reviewboard.mozilla.org/r/156694/#review163704

thanks. good changes.
Pushed by mcmanus@ducksong.com:
https://hg.mozilla.org/integration/autoland/rev/e0e7ef158af2
Implement the secureConnectionStart property for the PerformanceTiming interface r=bkelly,dragana,francois,Honza
https://hg.mozilla.org/mozilla-central/rev/e0e7ef158af2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I've updated the support information on:

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceTiming
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceTiming/secureConnectionStart

I've also add a note to the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56#DOM

Weird though - as the docs were already written, and said it had been implemented in Fx since version 7? Typo or copy&paste error possibly.
Depends on: 1384679
Assignee: nobody → mcmanus
Depends on: 1390839
You need to log in before you can comment on or make changes to this bug.