Closed Bug 1578991 Opened 3 months ago Closed 2 months ago

9.17 - 16.31% raptor-tp6-twitch-firefox (windows10-64-shippable) regression on push b3ee5fed44ec711f79202150223cda9397c6ed80 (Wed August 14 2019)

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set

Tracking

(firefox-esr60 unaffected, firefox-esr68 unaffected, firefox69 unaffected, firefox70 disabled, firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- disabled
firefox71 --- fixed

People

(Reporter: alexandrui, Assigned: sstreich)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: perf, perf-alert, regression, Whiteboard: [qf:tracking70])

Attachments

(1 file)

Raptor has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset= b3ee5fed44ec711f79202150223cda9397c6ed80

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

16% raptor-tp6-twitch-firefox windows10-64-shippable opt 132.95 -> 154.63
9% raptor-tp6-twitch-firefox windows10-64-shippable opt 129.99 -> 141.90

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=22871

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a Treeherder page showing the Raptor jobs in a pushlog format.

To learn more about the regressing test(s) or reproducing them, please see: https://wiki.mozilla.org/TestEngineering/Performance/Raptor

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Flags: needinfo?(sstreich)
Component: Performance → JSON Viewer
Product: Testing → DevTools
Target Milestone: --- → Firefox 71
Version: Version 3 → unspecified

This bisection revealed the regression.

Hey! 👋

Sorry for the regression.
The problem here is the Following:
Twitch.tv loads Documents with Nosniff enabled.
Prior to the Patch, we did not spin up any sniffer if nosniff was detected.
-> This broke the Json-Viewer - As this requires a special sniffer to be run in any case.

So the new patch now Lets each sniffer handle the nosniff flag by itself.
That also means we spin each sniffer up, even though it would always not have a result.

I will investigate this week if we can split the sniffers in "respects-nosniff" and !"respects-nosniff"
and call accordingly.

Assignee: nobody → sstreich
Flags: needinfo?(sstreich)
Whiteboard: [qf:tracking70]
Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb8d46c7826b
Skip Calling Sniffers in Case of NoSniff and JSON Mime r=ckerschb,dragana

Keywords: checkin-needed

It seems the patch only handles application/json, application/manifest+json and text/json.

But actually the JSON Viewer loads for any application/*+json (bug 1388335), e.g. application/vnd.api+json, application/hal+json or application/ld+json. I guess these will break with NoSniff.

And the JSON Viewer doesn't load for text/json.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(sstreich)
Regressions: 1583869

Please fix bug 1583869 before uplifting this to beta.

Fx70 RC week is next week. If this is going to make it into 70, it needs an uplift request ASAP. Otherwise we should wontfix.

Hey! I just proposed 1585055 for uplift.
If this one gets uplifted - Nosniff will be disabled in 70 which is the root cause for this perf-regression, so we wont run into this unless a user manually activates it. So i guess this does not need an uplift and we can mark this as fixed :)

Flags: needinfo?(sstreich)
See Also: → 1585055

Hi Sebastian, is there something that we can manually verify here? Thanks.

Flags: needinfo?(sstreich)

Hey! The patch here just skips the sniffing in a few cases where sniffing would not make sense to do anyway.
I'm not sure if this is easy to manually verify.

Flags: needinfo?(sstreich)

Oh, ok then, thanks for the fast reply!

You need to log in before you can comment on or make changes to this bug.