1762482 - Update `PerfomanceMeasure` APIs to User Timing L3

Status: RESOLVED FIXED

(Core :: DOM: Performance, enhancement)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Like bug 1724645 but updating the PerformanceMeasure APIs to User Timing L3.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1762482 - update PerformanceMeasure to User Timing L3. r=sefeng

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1762482 - make performance-measure-invalid.worker.js test pass. r=sefeng

Our code correctly threw an exception but it was of the wrong type because we
weren't explicitly checking the error case mentioned in the spec.

In the existing code, IsPerformanceTimingAttribute's had two responsibilities:

• return true/false if the given string is a perf timing attribute
• (via inheritance) but only return true if the global is a Window

This logic worked with the old version of the spec but doesn't always work with
User Timing L3 so I split this logic into separate methods, which I also feel is
more composable, explicit, and intuitive (e.g. if I call a method named
IsPerformanceTimingAttribute, I expect it to tell me if the string is a
performance timing attribute, not whether the method is being called on the
appropriate global).

Depends on D143858

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This bug and bug 1724645 can be implemented in any order. However, my code is built on my patch from bug 1724645 which I suspect will cause a merge conflict if they land in the opposite order so I'm marking these bugs as a dependency.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1762482 - add WPT for performance.measure('name', {detail: null}); r=sefeng

In "3.1.3 measure() method", we should run the following checks if start, end,
duration, and detail are present. I understand passing {detail: null} to mean
detail is present. This will cause check 2 to fail, which is what this test is
testing.

Depends on D143938

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I requested landing. I am aware we are 2 days from Beta merge and it's a Friday but this functionality pairs well with bug 1724645 which landed in this version of Firefox and I'd like to get them in the same version.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cf1c157d1ef
update PerformanceMeasure to User Timing L3. r=sefeng,smaug

Comment 8

[Marian-Vasile Laza]

Backed out changeset 3cf1c157d1ef (Bug 1762482) for causing bc failures on browser_cross_origin_isolated_performance_api.js.
Backout link
Push with failures <--> bc6
Failure Log

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I was able to reproduce this locally. I found the following error in the logs:

[task 2022-04-29T21:26:30.576Z] 21:26:30     INFO - Console message: [JavaScript Error: "SyntaxError: Performance.measure: Given mark name, Test-End, is unknown" {file: "https://example.com/browser/browser/components/resistfingerprinting/test/browser/coop_header.sjs?crossOriginIsolated=true&worker=true line 10 > eval" line: 1}]
[task 2022-04-29T21:26:30.576Z] 21:26:30     INFO - @https://example.com/browser/browser/components/resistfingerprinting/test/browser/coop_header.sjs?crossOriginIsolated=true&worker=true line 10 > eval:1:13
[task 2022-04-29T21:26:30.576Z] 21:26:30     INFO - self.onmessage@https://example.com/browser/browser/components/resistfingerprinting/test/browser/coop_header.sjs?crossOriginIsolated=true&worker=true:10:10

And here's the line where it's called: https://searchfox.org/mozilla-central/rev/6d3bc9da27904dbf6f6defda81019916235b8dd5/browser/components/resistfingerprinting/test/browser/browser_cross_origin_isolated_performance_api.js#49 I'll have to investigate why this mark could not be found because it looks like it should be.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The root cause of the failure is that we changed the behavior of Performance::Measure when ShouldResistFingerprinting == true such that we'll execute the entire Performance::Measure method except for InsertUserEntry(PerformanceMeasure). However, since there are no marks added to the buffer if we're resisting fingerprinting and Performance::Measure will throw an error if a named mark is missing, we end up throwing an error most of the time when Performance::Measure is called. To avoid throwing in the test (and presumably most user code on the internet), I changed the code to return a dummy PerformanceMeasure if we're resisting fingerprinting instead of proceeding through the method and just skipping adding the measure to the buffer.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1762482 - add performance.mark/measure to *reduce_time_precision tests. r=sefeng

These methods generate timestamps so we should verify they have reduced time
precision.

Depends on D144866

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I fixed the test failure and this is ready to land though I'm going to wait until after the branch cut on 5/30.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d93cc964b79
update PerformanceMeasure to User Timing L3. r=sefeng,smaug

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2d93cc964b79

Comment 15

[Sean Feng [:sefeng]]

I think this worth an ESR uplift as this fixes the google translate is broken is ESR issue (bug 1806957)

Comment 16

[Sean Feng [:sefeng]]

Comment on attachment 9272557 [details]
Bug 1762482 - update PerformanceMeasure to User Timing L3. r=sefeng

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Users may not be able to use Google Transalte
• Fix Landed on Version: 103.0a1
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is not risky as it upgrades the existing User Timing API to a new version but also keep it backward compatible with existing code.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9272557 [details]
Bug 1762482 - update PerformanceMeasure to User Timing L3. r=sefeng

Not normally the kind of thing we'd like to backport to an ESR, but I agree that having broken Google Translate is pretty bad. Approving for 102.7esr since this has been enabled by default in Release since 103 without any major regressions, grafts cleanly, and has good test coverage and backwards compatibility.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/d332ceb003cf