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)
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)
4.79 KB,
patch
|
dragana
:
review+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•6 years ago
|
||
I did some extra testing: Works with Firefox 55, doesn't work with 56 and 57.
Updated•6 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
Comment 2•6 years ago
|
||
Can you please use https://mozilla.github.io/mozregression/ to determine the regression range?
Keywords: regression,
regressionwindow-wanted
Comment 3•6 years ago
|
||
secureConnectionStart property was implemented in bug 772589. Patrick may know what's up here.
Flags: needinfo?(mcmanus)
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(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
Reporter | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
taking a look.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Assignee | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6ea8151c74e544cbdcf02552983e916ecd30c81
Comment 10•6 years ago
|
||
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-
Assignee | ||
Comment 11•6 years ago
|
||
(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 12•6 years ago
|
||
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+
Assignee | ||
Comment 13•6 years ago
|
||
Attachment #8931329 -
Attachment is obsolete: true
Attachment #8933684 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa94e8379dcffc587b8f3a2045f5fb8d6e44940
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=080508d6dceb294f36c3d7cb2c037a882215bdc3
Assignee | ||
Comment 16•6 years ago
|
||
update a test to reflect the new behavior.
Attachment #8933684 -
Attachment is obsolete: true
Attachment #8934474 -
Flags: review+
Comment 17•6 years ago
|
||
thanks
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e4bfa4312745
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
Comment 20•6 years ago
|
||
[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.
Comment 21•6 years ago
|
||
Track 58+ as this is required by bug 1424341.
Comment 22•6 years ago
|
||
Hi Tom, since this is required by bug 1424341, please nominate beta uplift request.
Flags: needinfo?(rkothari) → needinfo?(tom)
Comment 23•6 years ago
|
||
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 24•6 years ago
|
||
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+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 25•6 years ago
|
||
bugherder uplift |
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)
Comment 26•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/fee5a8fb1f455dd8459b836e706613ebf49a93dc (FIREFOX_58b_RELBRANCH) https://hg.mozilla.org/releases/mozilla-release/rev/494218c56c7eb448d61c1c803342881f27686ca7
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/fee5a8fb1f455dd8459b836e706613ebf49a93dc (FIREFOX_58b_RELBRANCH)
Comment 27•6 years ago
|
||
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.
Assignee | ||
Comment 28•6 years ago
|
||
(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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•