Closed
Bug 772589
Opened 12 years ago
Closed 7 years ago
Implement the secureConnectionStart property for the PerformanceTiming interface
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: mcmanus)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file)
This new property should use the new DOM bindings (see the patch in bug 749101).
Reporter | ||
Comment 1•12 years ago
|
||
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
Comment 2•12 years ago
|
||
This is the only part of the Navigation Timing test suite we fail...
Assignee | ||
Comment 3•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8885914 -
Flags: review?(tnguyen)
Attachment #8885914 -
Flags: review?(dd.mozilla)
Attachment #8885914 -
Flags: review?(bkelly)
Assignee | ||
Comment 5•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8885914 -
Flags: review?(tnguyen) → review?(ntim.bugs)
Comment 6•7 years ago
|
||
mozreview-review |
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)
Updated•7 years ago
|
Attachment #8885914 -
Flags: review?(odvarko)
Comment 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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.
Comment 11•7 years ago
|
||
(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 12•7 years ago
|
||
(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...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2c974e859d5aaecd8c259b743afe1b945c32a0a&selectedJob=115182556
thanks for the pointer to the har format - i changed that to ssl
Assignee | ||
Updated•7 years ago
|
Attachment #8885914 -
Flags: review?(francois)
Assignee | ||
Comment 18•7 years ago
|
||
: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 19•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
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.
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 24•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee: nobody → mcmanus
You need to log in
before you can comment on or make changes to this bug.
Description
•