1435434 - Use script elements instead of loadSubScript

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Andrew McCreight [:mccr8]]

There are a few files that use enablePrivilege entirely so they can call Services.scriptloader.loadSubScript() on TalosContentProfiler.js. However, I think they could just load that script using a script element, in content.

This seems to include:
  pageloader/chrome/tscroll.js
  tests/gfx/benchmarks/rasterflood_gradient.html
  tests/gfx/benchmarks/rasterflood_svg.html
  layout/benchmarks/displaylist_mutate.htm
  tests/svgx/hixie-00*.xml

I'll have to make sure that --geckoProfiler still works after I do that.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1435434 - remove usage of enablePrivilege in talos which was solely for calling loadSubScript;

Review commit: https://reviewboard.mozilla.org/r/218732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218732/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Local talos runs look ok, if anyone knows what the right set of builds to trigger to cover all the talos tests are, I'll kick off a try build to confirm I didn't miss anything.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8949393 [details]
Bug 1435434 - remove usage of enablePrivilege in talos which was solely for calling loadSubScript;

https://reviewboard.mozilla.org/r/218732/#review224518

this looks good, can we run all the tests:
./mach try -b o -p linu64 -u none -t all

I don't think there would be any differences on osx/windows; this would ensure all runs ok.  As for ensuring we still generate valid profiles, we should check that.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

:rwood, can you advice on how to ensure we get geckoProfile for all tests, ideally on try?

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9d097f1e044f8dd213dc4e0738e4b61574af7b

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Let's try that again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e82b569920ea42d830c6242a0efd31ea1c3fa3

Comment 7

[Robert Wood [:rwood]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #4)
> :rwood, can you advice on how to ensure we get geckoProfile for all tests,
> ideally on try?

Sure, just add 'mozharness: --geckoProfile' to the end of your try syntax i.e.
 
try: -b o -p linux64 -u none -t all mozharness: --geckoProfile --artifact

Then in treeherder on your try run, select one of the completed talos job symbols, and click on the 'Job Details' tab. You'll see at least one 'profile_<talostest>.zip' artifact uploaded (one per talos test in the suite).

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Hmm, so it doesn't seem to be happy with the hixie-*.svg changes; the TalosContentProfiler name isn't defined. I don't see any errors in the browser console about the load failing, but maybe they don't show. Are there any special rules about loading scripts from SVGs?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I am not too familiar with SVG syntax or restrictions

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8949393 [details]
Bug 1435434 - remove usage of enablePrivilege in talos which was solely for calling loadSubScript;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218732/diff/1-2/

Comment 11

[Andrew McCreight [:mccr8]]

Daniel, do you have any suggestions for making the svg changes work?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Are there any special rules about loading scripts from SVGs?

None that I'm aware of that'd come into play here, but maybe jwatt might have some idea about what's going on...

Comment 13

[Daniel Holbert [:dholbert]]

Oh! So this change has stuff like...
> + <script type="text/javascript" src="chrome://talos-powers-content/content/TalosContentProfiler.js"></script>

...but IIRC SVG uses "xlink:href" attribute for scripts rather than the "src" attribute, for Reasons.

So I think you need to:
 (1) replace src with xlink:href above
 (2) edit the <svg> element at the top of this file to reference the xlink:href namespace, by adding...
       xmlns:xlink="http://www.w3.org/1999/xlink"
    ...to the <svg> element, as in e.g. this file:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/title_test.svg

If that doesn't work, I'd suggest needinfo'ing jwatt.

Comment 14

[Daniel Holbert [:dholbert]]

For reference/comparison, here's an SVG file in the tree, which uses <script xlink:href="..."> (and which has the xlink namespace defined on the svg node to support that usage):
https://dxr.mozilla.org/mozilla-central/source/layout/reftests/svg/image/image-preserveAspectRatio-01-raster.svg

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Yup, that was the correct magic. try is still showing a timeout on scroll, which I'm still attempting to reproduce locally.

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8949393 [details]
Bug 1435434 - remove usage of enablePrivilege in talos which was solely for calling loadSubScript;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218732/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c6f14a7398b
remove usage of enablePrivilege in talos which was solely for calling loadSubScript; r=jmaher

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1c6f14a7398b

Comment 19

[Ionuț Goldan [:igoldan]]

Looks like this bug updated our baselines:

== Change summary for alert #11568 (as of Mon, 12 Feb 2018 14:40:43 GMT) ==

Improvements:

  2%  tsvgx windows7-32 pgo e10s stylo     388.46 -> 379.76
  1%  tsvgx windows7-32 opt e10s stylo     448.01 -> 444.74

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11568