Closed Bug 1417431 Opened 6 years ago Closed 6 years ago

secureConnectionStart value in the navtimings is not correct

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: simon.schatka, Assigned: dragana)

References

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.89 Safari/537.36

Steps to reproduce:

I opened a page that does not use https(I used http://orf.at, but i guess it does not matter) and then had a look at the navtiming information


Actual results:

Here is where it gets interesting:
I typed "performance.timing" in the console and it returns this:
"PerformanceTiming { navigationStart: 1510752442908, unloadEventStart: 0, unloadEventEnd: 0, redirectStart: 0, redirectEnd: 0, fetchStart: 1510752442908, domainLookupStart: 1510752442915, domainLookupEnd: 1510752442915, connectStart: 1510752442915, connectEnd: 1510752442921 }"
Note: There is no secureConnectionStart value set(which is what is expected)

But, then I accessed the value directly: "performance.timing.secureConnectionStart" and it returns "1510752442908", which is the same as navigationStart(which is wrong)

When I show the values of the object from the "performance.timing" call in the value view, then I get a long list of values suddenly: 
connectEnd:1510752442921
connectStart:1510752442915
domComplete:1510752444083
domContentLoadedEventEnd:1510752443327
domContentLoadedEventStart:1510752443314
domInteractive:1510752443311
domLoading:1510752442939
domainLookupEnd:1510752442915
domainLookupStart:1510752442915
fetchStart:1510752442908
loadEventEnd:1510752444083
loadEventStart:1510752444083
navigationStart:1510752442908
redirectEnd:0
redirectStart:0
requestStart:1510752442921
responseEnd:1510752442949
responseStart:1510752442936
secureConnectionStart:1510752442908
unloadEventEnd:0
unloadEventStart:0


Expected results:

According to https://www.w3.org/TR/navigation-timing/ secureConnectionStart should be set to 0 if https is not used:
"This attribute is optional. User agents that don't have this attribute available must set it as undefined. When this attribute is available, if the scheme of the current page is HTTPS, this attribute must return the time immediately before the user agent starts the handshake process to secure the current connection. If this attribute is available but HTTPS is not used, this attribute must return zero"

I am also not sure why the console is showing only a part of the object when I call performance.timing, but when i retrieve the values directly I get a value?
I did some extra testing:
Works with Firefox 55, doesn't work with 56 and 57.
Component: Untriaged → DOM
Product: Firefox → Core
Can you please use https://mozilla.github.io/mozregression/ to determine the regression range?
secureConnectionStart property was implemented in bug 772589. Patrick may know what's up here.
Flags: needinfo?(mcmanus)
(In reply to simon.schatka from comment #1)
> I did some extra testing:
> Works with Firefox 55, doesn't work with 56 and 57.

I'm surprised it worked in 55 when it seems it wasn't implemented until 56 (in bug 772589). Maybe Dragana (one of the reviewers of bug 772589) has thoughts as it seems Patrick may be out.
Flags: needinfo?(dd.mozilla)
Priority: -- → P2
You are right. I did the test with firefox 55 and it is not present there. I noticed the problem because my tests were failing with firefox 56 and did the extensive test there. I just assumed it was working correctly with ff55, because my tests were not failing(the secureConnectionStart value was not available, but was defaulted to 0, which is what my tests were expecting)

So to summarize: Not a regression bug, but a bug in a new feature.
taking a look.
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Thanks for confirming, Simon!
Attached patch bug_1417431_v1.patch (obsolete) — Splinter Review
The relevant part of the spec:
"
secureConnectionStart attribute

This attribute is optional. User agents that don't have this attribute available must set it as undefined. When this attribute is available, if the scheme of the current page is HTTPS, this attribute must return the time immediately before the user agent starts the handshake process to secure the current connection. If this attribute is available but HTTPS is not used, this attribute must return zero. 
"
Attachment #8931329 - Flags: review?(bkelly)
Comment on attachment 8931329 [details] [diff] [review]
bug_1417431_v1.patch

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

Can you please investigate why we are getting a non-null value from GetSecureConnectionStart()?

::: dom/performance/PerformanceMainThread.cpp
@@ +79,5 @@
> +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> +    nsCOMPtr<nsIURI> uri;
> +    httpChannel->GetURI(getter_AddRefs(uri));
> +    bool isHttps = false;
> +    nsresult rv = uri->SchemeIs("https", &isHttps);

Can you calculate the isHttps boolean in PerformanceTiming instead of requiring it to be passed?  We already pass the channel so I think that would be an easy way to de-duplicate this code from the different call sites.

::: dom/performance/PerformanceTiming.cpp
@@ +22,5 @@
>  
>  PerformanceTiming::PerformanceTiming(Performance* aPerformance,
>                                       nsITimedChannel* aChannel,
>                                       nsIHttpChannel* aHttpChannel,
> +                                     bool aIsHttps,

Please calculate this from aHttpChannel.

@@ +372,5 @@
>        nsContentUtils::ShouldResistFingerprinting()) {
>      return mZeroTime;
>    }
> +  return !mSecureConnection
> +    ? 0

This should be mZeroTime, no?

Also, I think it might be easier to force mSecureConnectionStart to the null TimeStamp() value if !mSecureConnection.

Also, do we know why nsITimedChannel::GetSecureConnectionStart() is returning a value other than TimeStamp() when TLS is not being used?  That seems like a bug further down the stack to me.
Attachment #8931329 - Flags: review?(bkelly) → review-
(In reply to Ben Kelly [:bkelly] from comment #10)
> Comment on attachment 8931329 [details] [diff] [review]
> bug_1417431_v1.patch
> 
> Review of attachment 8931329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please investigate why we are getting a non-null value from
> GetSecureConnectionStart()?
> 
> ::: dom/performance/PerformanceMainThread.cpp
> @@ +79,5 @@
> > +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> > +    nsCOMPtr<nsIURI> uri;
> > +    httpChannel->GetURI(getter_AddRefs(uri));
> > +    bool isHttps = false;
> > +    nsresult rv = uri->SchemeIs("https", &isHttps);
> 
> Can you calculate the isHttps boolean in PerformanceTiming instead of
> requiring it to be passed?  We already pass the channel so I think that
> would be an easy way to de-duplicate this code from the different call sites.
> 
> ::: dom/performance/PerformanceTiming.cpp
> @@ +22,5 @@
> >  
> >  PerformanceTiming::PerformanceTiming(Performance* aPerformance,
> >                                       nsITimedChannel* aChannel,
> >                                       nsIHttpChannel* aHttpChannel,
> > +                                     bool aIsHttps,
> 
> Please calculate this from aHttpChannel.
> 
> @@ +372,5 @@
> >        nsContentUtils::ShouldResistFingerprinting()) {
> >      return mZeroTime;
> >    }
> > +  return !mSecureConnection
> > +    ? 0
> 
> This should be mZeroTime, no?
> 

mZeroTime is not 0 but navigation start time :)

> Also, I think it might be easier to force mSecureConnectionStart to the null
> TimeStamp() value if !mSecureConnection.

In all my tests  mSecureConnectionStart was null if tls is not used.
I can also make this check in PerformanceTiming::InitializeTimingInfo.

> 
> Also, do we know why nsITimedChannel::GetSecureConnectionStart() is
> returning a value other than TimeStamp() when TLS is not being used?  That
> seems like a bug further down the stack to me.

because mZeroTime is a very stupid name for something that is not zero but it is set in PerformanceTiming constructor and it is set to navigation start time :)

so:
return mSecureConnectionStart.IsNull() ? mZeroTime : TimeStampToDOMHighRes(mSecureConnectionStart);

will never return 0.
Comment on attachment 8931329 [details] [diff] [review]
bug_1417431_v1.patch

Ok, I understand now.  Maybe add a comment clarifying why we are using 0 and mZeroTime in the same expression.

r=me with other comments addressed.  Thanks!
Attachment #8931329 - Flags: review- → review+
Attached patch bug_1417431_v1.patch (obsolete) — Splinter Review
Attachment #8931329 - Attachment is obsolete: true
Attachment #8933684 - Flags: review+
Flags: needinfo?(mcmanus)
update a test to reflect the new behavior.
Attachment #8933684 - Attachment is obsolete: true
Attachment #8934474 - Flags: review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4bfa4312745
secureConnectionStart should be 0 for pages with HTTP scheme. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/e4bfa4312745
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
[Tracking Requested - why for this release]: While not strictly required for Bug 1424341 to land, this is a cleaner implementation to land on top of. 

Patch applies cleanly to -beta for me.
Blocks: 1424341
Flags: needinfo?(rkothari)
Track 58+ as this is required by bug 1424341.
Hi Tom, since this is required by bug 1424341, please nominate beta uplift request.
Flags: needinfo?(rkothari) → needinfo?(tom)
Comment on attachment 8934474 [details] [diff] [review]
bug_1417431_v1.patch

Approval Request Comment
[Feature/Bug causing the regression]: We would like the ability to easily tune timer precision on beta for the next release cycle to respond to Spectre. The pref is turned off by default right now.

[User impact if declined]: We will have to do dot releases instead of doing a system addon to tweak a pref. Also, the dot releases will be more work (much more work if we don't uplift this in one of them) to cover all timers - previously our chemspill was only to address performance.now()

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: This is a dependency for 1424341

[Is the change risky?]: No

[Why is the change risky/not risky?]: This is a small patch that determines if a connection is HTTPS and if not, returns zero for a performance property.

[String changes made/needed]: None
Flags: needinfo?(tom)
Attachment #8934474 - Flags: approval-mozilla-beta?
Comment on attachment 8934474 [details] [diff] [review]
bug_1417431_v1.patch

Required by bug 1424341. Beta58+.
Attachment #8934474 - Flags: approval-mozilla-release+
Attachment #8934474 - Flags: approval-mozilla-beta?
Attachment #8934474 - Flags: approval-mozilla-beta+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/fee5a8fb1f45
Keywords: checkin-needed
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/fee5a8fb1f455dd8459b836e706613ebf49a93dc (FIREFOX_58b_RELBRANCH)
Another problem is that secureConnectionStart should be greater than connectStart whenever it is set. See the sample diagram in https://www.w3.org/TR/navigation-timing/#processing-model. The definitions can also be found in that document. I'm trying to land a new web platform test for these attributes but I'm blocked on Firefox nightly being flaky, see https://github.com/w3c/web-platform-tests/pull/9040.
(In reply to npm from comment #27)
> Another problem is that secureConnectionStart should be greater than
> connectStart whenever it is set. See the sample diagram in
> https://www.w3.org/TR/navigation-timing/#processing-model. The definitions
> can also be found in that document. I'm trying to land a new web platform
> test for these attributes but I'm blocked on Firefox nightly being flaky,
> see https://github.com/w3c/web-platform-tests/pull/9040.

I will open a separate bug for this issue. I will take a look why test_performance_attributes.sub.html is failing.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.