Closed Bug 1462883 Opened 2 years ago Closed 2 years ago

Performance object must be reset when the inner window changes document

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

In nsGlobalWindowInner::InnerSetNewDocument and in nsGlobalWindowOuter::SetNewDocument we must set mPerformance to null in order to create a fresh new object when needed. This new object will have a correct PerformanceNavigationTiming entry.

I suspect that this patch must be uplift to beta/esr when reviewed.
Attached patch performance4.patch (obsolete) — Splinter Review
Attachment #8979006 - Flags: review?(bzbarsky)
Why this would need to be uplifted? Is this a regression?
Attachment #8979006 - Attachment is obsolete: true
Attachment #8979006 - Flags: review?(bzbarsky)
Attachment #8979178 - Flags: review?(bzbarsky)
Comment on attachment 8979178 [details] [diff] [review]
performance4.patch

>+  // This must be called after nullifying the internal objects because here we
...
>+  // slots. If we nullify them after, the slot values and the objects will be

s/nullifying/nulling out/ and s/nullify them after/null them out later/.  Both places.

We should try to make sure we have tests for both codepaths there and check whether our implementation matches the other browsers...  Followup is OK for that.
Attachment #8979178 - Flags: review?(bzbarsky) → review+
> We should try to make sure we have tests for both codepaths there and check
> whether our implementation matches the other browsers...  Followup is OK for
> that.

WPT passes on chrome. I haven't checked other browsers.
The WPT covers nsGlobalWindowInner::InnerSetNewDocument, but not nsGlobalWindowOuter::SetNewDocument.
What should I do to test this second path?
Flags: needinfo?(bzbarsky)
Something like this:

  <iframe></iframe>
  <script>
    var perf = frames[0].performance;
    frames[0].location = someSameOriginURI;
  </script>

should exercise that path, I would think.  You basically want to hit the "reUseInnerWindow" case in nsGlobalWindowOuter::SetNewDocument.
Flags: needinfo?(bzbarsky)
Attached patch testSplinter Review
Attachment #8979940 - Flags: review?(annevk)
Attachment #8979940 - Flags: review?(annevk) → review+
Attached patch WPT disabled (obsolete) — Splinter Review
Fixing this issue, a new one appeared: requestStart is lower than what expected by these tests. I'm planning to work on them in a follow up.
bz, are you OK with it?
Attachment #8980054 - Flags: review?(bzbarsky)
Comment on attachment 8980054 [details] [diff] [review]
WPT disabled

Wait, why does this change introduce new failures in tests that used to be passing, exactly?  That seems strange.  I would really like us to understand why, because the simplest explanation is that this change just doesn't match the spec and then we probably shouldn't do it...
Flags: needinfo?(amarchesini)
Attachment #8980054 - Flags: review?(bzbarsky)
Depends on: 1464135
Flags: needinfo?(amarchesini)
Priority: -- → P2
Attached patch performance4bis.patch (obsolete) — Splinter Review
I finally found the time to check why my patch was breaking a WPT.
The reason is that, before this set of patches, Performance object was not updated when the window was setting a new Document. We were basically using the Performance object generated for about:blank. That was having PerformanceTimingData::mReportCrossOriginRedirect set to false, and the test passed.

With my patches, PerformanceTimingData is correctly created when a new document is set. But, for how the code was written, mReportCrossOriginRedirect was set only if we had a httpChannel available. At that point (at the document creation) the httpChannel is not available (I haven't check why...) and mReportCrossOriginRedirect was left to its default value: true.

In bug 1462889, I'm going to change how Performance object is updated, but this must land first.
Attachment #8980054 - Attachment is obsolete: true
Attachment #8996376 - Flags: review?(bzbarsky)
Comment on attachment 8996376 [details] [diff] [review]
performance4bis.patch

I don't understand why this is right.  We used to SetPropertiesFromHttpChannel() in the navigation case; now we won't do that any more.

But more importantly, why are we not passing the nsIHttopChannel in the  navigation case?  If it's just to avoid the CheckAllowedOrigin() thing, then shouldn't we condition just that on whether this is a navigation or not?  I don't see why we should skip setting these various other members in that case, and the documentation in the code doesn't seem to help much with understanding what's going on here.
Flags: needinfo?(valentin.gosu)
Attachment #8996376 - Flags: review?(bzbarsky)
I don't think we did anything to intentionally avoid CheckAllowedOrigin().
Actually, I would love it if we could get rid of EnsureDocEntry and just leave CreateDocumentEntry.
From what I can tell from the patch, moving the SetPropertiesFromHttpChannel bits to the PerformanceTimingData constructor is actually a good move.
Flags: needinfo?(valentin.gosu)
Well, except we are no longer calling it, then....
Attachment #8996376 - Attachment is obsolete: true
Attachment #8996933 - Flags: review?(bzbarsky)
Comment on attachment 8996933 [details] [diff] [review]
part 3 - update mReportCrossOriginRedirect

This makes a lot more sense to me, thank you!

Though I still wonder whether it would be cleaner to just pass the HTTP channel to the constructor and have some other signal for the constructor's "is a navigation" conditions.  We can do that in a separate bug, though.
Attachment #8996933 - Flags: review?(bzbarsky) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6577b8c72f
Performance object must be reset when the inner window changes document, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d72ac32f81c
Performance object must be reset when the inner window changes document - WPT, r=annevk
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bfaea2aa07
Update PerformanceTimingData::mReportCrossOriginRedirect in SetPropertiesFromHttpChannel, r=bz
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12294 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/ea6577b8c72f
https://hg.mozilla.org/mozilla-central/rev/8d72ac32f81c
https://hg.mozilla.org/mozilla-central/rev/93bfaea2aa07
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.