Closed
Bug 1099092
Opened 10 years ago
Closed 9 years ago
Navigation Timing has incorrect values when page is load via link with target=_blank attribute
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: peters, Assigned: valentin)
References
(Blocks 1 open bug)
Details
(Keywords: reproducible, testcase)
Attachments
(4 files, 5 obsolete files)
1.16 KB,
text/html
|
Details | |
2.48 KB,
patch
|
Details | Diff | Splinter Review | |
5.52 KB,
patch
|
Details | Diff | Splinter Review | |
5.20 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36 Steps to reproduce: Click on any link wich has target="_blank" attribute and check values of performance.timing object. Actual results: Absolutely all values in perforamce.timing are zero or equal to navigationStart value. Expected results: Most of values (especially DOM events timings) should not be zero or should be at least greater then navigationStart value.
Attachment #8522947 -
Attachment is obsolete: true
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: reproducible,
testcase
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Comment 2•10 years ago
|
||
Valentin, do you have time to take a look here?
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 4•9 years ago
|
||
I tracked it down to this https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6614d8d85f9&tochange=7f81be7db528 The most likely culprit seems to be https://hg.mozilla.org/mozilla-central/rev/d1b960226e0b Basically, what happens is that a DOMNavigationTiming is created for performance.timing, but when that gets nulled out and a new one is created, only the new one gets the updated timestamps and events. I'm guessing that if we call mTiming->Clear() on the object, instead of creating a new one, that would fix the issue. I'll look into it.
Flags: needinfo?(valentin.gosu)
Comment 5•9 years ago
|
||
Hmm. The nulling out was to preserve preexisting behavior. When we create the new mTiming, shouldn't we also associate it with a new or existing performance.timing? It's not clear to me what the spec says here....
Assignee | ||
Comment 6•9 years ago
|
||
This seems to work, but I'm not entirely sure this is the correct way to set the timing object on the window.
Attachment #8534728 -
Flags: review?(bzbarsky)
Comment 7•9 years ago
|
||
Comment on attachment 8534728 [details] [diff] [review] Navigation Timing has incorrect values when page is load via link with target=_blank attribute mBlankTiming never gets set to false. I don't see how that can possibly be correct... Need more tests, clearly. :(
Attachment #8534728 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8535330 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•9 years ago
|
||
You are right. Also, that patch had bad values for unloadStart/End because I was creating a new timing for the window. I also found another error regarding redirectStart/End which was my fault (from the resource timing bug). The new approach seems a little better to me, and I added a mochitest based on the testcase, which verifies the simple load, _blank and _self cases.
Attachment #8535332 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Attachment #8534728 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
Comment on attachment 8535332 [details] [diff] [review] Navigation Timing has incorrect values when page is load via link with target=_blank attribute So if we land in MaybeInitTiming when mBlankTiming but !mScriptGlobal... won't we end up crashing on the null-deref when we do mTiming->NotifyNavigationStart()?
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 11•9 years ago
|
||
That's correct. Is that a common case? I haven't seen any crashes on try.
How about
> if (!mBlankTiming || !mScriptGlobal) {
> mTiming = new nsDOMNavigationTiming();
Flags: needinfo?(valentin.gosu)
Comment 12•9 years ago
|
||
Comment on attachment 8535330 [details] [diff] [review] Add tests for navigation timing >+ for (var i in timingParams) { >+ var name = timingParams[i]; for (var name of timingParams) { (both places). >+function test_blank() { Why is the setTimeout call here needed? Document. And explicitly pass 0 as the timeout value? r=me >+function test_self() { Same thing here. >+function test_load() { And here.
Attachment #8535330 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Also a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8847ef9dd008
Attachment #8538465 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Attachment #8535332 -
Attachment is obsolete: true
Attachment #8535332 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8535330 -
Attachment is obsolete: true
Comment 16•9 years ago
|
||
Comment on attachment 8538465 [details] [diff] [review] Navigation Timing has incorrect values when page is load via link with target=_blank attribute >+ bool mBlankTiming; Needs a comment describing what it means. Past that, seems reasonable, I think. I assume you've tested both cases that reuse the inner window and ones that don't?
Attachment #8538465 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8538465 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #16) > Past that, seems reasonable, I think. I assume you've tested both cases > that reuse the inner window and ones that don't? I did a manual test, and it seems to behave correctly.
Comment 19•9 years ago
|
||
What were the manual steps you used?
Assignee | ||
Comment 20•9 years ago
|
||
I opened a page and checked window.performance.timing Clicked on a link (same domain) and checked that the timing info is different Clicked the Back button, and checked that the timing info is the same as the first one. Refreshed, checked that the timing info has changed. Clicked on a link (other domain) and checked that the timing info has changed (and unloadEvent timings are 0) Opening links with _blank and _self is tested in the mochitest.
Comment 21•9 years ago
|
||
OK. Here's what I meant in comment 16. 1) Page opens a window with window.open() but no url, then later sets location of that window to a same-origin URL. Want to make sure that the timing is sane both before the location set and after. 2) Page opens a window with window.open() but no url, then later sets location of that window to a cross-origin URL. Want to make sure that the timing is sane both before the location set and after. 3) A link with target=_blank, pointing to a same-origin URL; check timing after page finishes loading. 4) A link with target=_blank, pointing to a cross-origin URL; check timing after page finishes loading.
Assignee | ||
Comment 22•9 years ago
|
||
I checked these cases as well, and everything seems fine.
Comment 23•9 years ago
|
||
Great, thank you!
Assignee | ||
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d5cd863b14 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0341b791d11 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4d06a8f2ca
Assignee | ||
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15e6350d86b
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f2d5cd863b14 https://hg.mozilla.org/mozilla-central/rev/f0341b791d11 https://hg.mozilla.org/mozilla-central/rev/7a4d06a8f2ca https://hg.mozilla.org/mozilla-central/rev/f15e6350d86b
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
•