1405349 - Refactor reftest manifest parsing code into a separate JSM

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Andrew Halberstadt [:ahal]]

The JavaScript portion of the reftest harness lives mostly in a single large file (reftest.jsm). It handles everything in the chrome side, from parsing manifests, to loading tests, to capturing logs and results + much more. It also makes very heavy use of globals for keeping track of state.

This pattern has made it very tricky to both understand reftest, and extend it without introducing subtle bugs. A worthy goal would be to A) separate logical pieces into several independent JSM's, and B) try to avoid relying on global state. A good first piece to factor out is all the manifest parsing logic. As a first iteration, I think it can still share global state with "reftest.jsm", and we can look to improve the situation in follow-up bugs.

This isn't just a refactor for refactors sake. The end goal is to support "run-by-manifest" mode in reftest (bug 1353461). But this particular bug seems worth doing either way.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

There are two motivations for this change.

First, reftest.jsm has become very large and monolithic. It has lots of global
state and is hard to modify without breaking something. This change is a first
attempt at dividing reftest.jsm into multiple standalone(ish) modules. This
will make it easier to comprehend and extend.

Second, we'd like to implement "run-by-manifest" mode for reftest. This means
we'll restart the browser between each manifest run. This means much of the
state which is normally stored in global variables in reftest.jsm, will instead
need to be stored in python. A prerequisite to doing that is parsing the
manifests before starting the test suite. A prerequisite to that is moving the
manifest parsing code into a standalone JSM. This is the first step.

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

Comment 2

[Andrew Halberstadt [:ahal]]

This patch looks good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66b0feef1467db5636142feddcf5266871364f72

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

Hey dbaron, no need to review this in depth (unless you want to), but wanted to make sure you were ok with this general approach. The commit message explains the motivation for this change.

Basically it pulls all manifest parsing related code into a manifest.jsm module. Since manifest parsing shares a decent amount of global state, it also pulls all global variables (and constants) into a globals.jsm module. I'm not sure if this is the best way to share state between the two modules or not though.

Thanks in advance.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

Yeah, I'm ok with the general approach as long as it's still possible to run the reftest harness without using the python parts; that's sometimes useful when working with debugging tools, or when doing interesting things (e.g., using it to screenshot CSSWG tests).

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

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

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #4)
> Comment on attachment 8915585 [details]
> Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to
> standalone module,
> 
> Yeah, I'm ok with the general approach as long as it's still possible to run
> the reftest harness without using the python parts; that's sometimes useful
> when working with debugging tools, or when doing interesting things (e.g.,
> using it to screenshot CSSWG tests).

Thanks, yeah this shouldn't change anything around that. The follow-up bug 1392391 might, but I'll keep this workflow in mind and try to make sure both cases still work.

Comment 7

[Andrew Halberstadt [:ahal]]

Here's the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a168dbd3f6d66e93c298c3030042c0f706260511

Comment 8

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

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

https://reviewboard.mozilla.org/r/186780/#review194106

a few nits below- overall this looks as though large chunks of variables and code are moved without any structural changes.  A few nits which could be addressed in a followup if there are actions to take.  Overall I do not like the use of 'g' as a variable, but it is easy to read in the modified code and do not think we should change it.

::: layout/tools/reftest/globals.jsm:19
(Diff revision 2)
> +  NS_GFXINFO_CONTRACTID: "@mozilla.org/gfx/info;1",
> +  IO_SERVICE_CONTRACTID: "@mozilla.org/network/io-service;1",
> +  DEBUG_CONTRACTID: "@mozilla.org/xpcom/debug;1",
> +  NS_LOCALFILEINPUTSTREAM_CONTRACTID: "@mozilla.org/network/file-input-stream;1",
> +  NS_SCRIPTSECURITYMANAGER_CONTRACTID: "@mozilla.org/scriptsecuritymanager;1",
> +  NS_REFTESTHELPER_CONTRACTID: "@mozilla.org/reftest-helper;1",

NS_REFTESTHELPER_CONTRACTID and NS_LOCALFILEINPUTSTREAM_CONTRACTID and NS_LOCAL_FILE_CONTRACTID isn't used at all.

::: layout/tools/reftest/globals.jsm:21
(Diff revision 2)
> +  DEBUG_CONTRACTID: "@mozilla.org/xpcom/debug;1",
> +  NS_LOCALFILEINPUTSTREAM_CONTRACTID: "@mozilla.org/network/file-input-stream;1",
> +  NS_SCRIPTSECURITYMANAGER_CONTRACTID: "@mozilla.org/scriptsecuritymanager;1",
> +  NS_REFTESTHELPER_CONTRACTID: "@mozilla.org/reftest-helper;1",
> +  NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX: "@mozilla.org/network/protocol;1?name=",
> +  NS_XREAPPINFO_CONTRACTID: "@mozilla.org/xre/app-info;1",

NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX and NS_XREAPPINFO_CONTRACTID and NS_SCRIPTSECURITYMANAGER_CONTRACTID and IO_SERVICE_CONTRACTID are only used in manifest.jsm.

::: layout/tools/reftest/globals.jsm:58
(Diff revision 2)
> +
> +  // "<!--CLEAR-->"
> +  BLANK_URL_FOR_CLEARING: "data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E",
> +
> +  RE_PROTOCOL: /^\w+:/,
> +  RE_PREF_ITEM: /^(|test-|ref-)pref\((.+?),(.*)\)$/,

this two RE_P* variables are only used in the new manifest.jsm?  should we move it there?  Also previously this was a const, should I assume that the lack of 'const' defaults to a 'const' value?

::: layout/tools/reftest/reftest.jsm:516
(Diff revision 2)
> -    gURIUseCounts = {};
> -    for (var i = 0; i < gURLs.length; ++i) {
> -        var url = gURLs[i];
> +    g.uriUseCounts = {};
> +    for (var i = 0; i < g.urls.length; ++i) {
> +        var url = g.urls[i];
>          if (url.expected != EXPECTED_DEATH &&
>              (url.type == TYPE_REFTEST_EQUAL ||
>               url.type == TYPE_REFTEST_NOTEQUAL)) {

these work ok in this patch, but any manifest parsing in multiple stages or different processes will present a challenge here.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

https://reviewboard.mozilla.org/r/186780/#review194106

I'm not crazy about using 'g' either. But in order for mutated variables to get updated across modules (i.e so if reftest.jsm modifies a variable, manifest.jsm will see it too), we need to use an object as that is passed by reference. Using a longer name would make the module less readable due to how often globals are used. There might be some kind of JS magic to avoid using a container object, but if there is it's beyond my knowledge.

> this two RE_P* variables are only used in the new manifest.jsm?  should we move it there?  Also previously this was a const, should I assume that the lack of 'const' defaults to a 'const' value?

I don't think it will be assigned with `const`. I'm not sure how to do that.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

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

Comment 11

[Andrew Halberstadt [:ahal]]

Here's one last sanity try push with the latest changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5ea88b4873d076fd30727e6561202a3b2b59ecf

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cf260e1fb9c
[reftest] Refactor manifest parsing from reftest.jsm to standalone module, r=jmaher

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/dade3202fe0d for https://treeherder.mozilla.org/logviewer.html#?job_id=136818642&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=136818745&repo=autoland

Comment 14

[Andrew Halberstadt [:ahal]]

So our current best idea is to move the Windows 7 debug non-e10s reftests to Windows 10. Things are a lot more stable over there. I'll file a new bug for this.

Comment 15

[Andrew Halberstadt [:ahal]]

Here's the latest try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be40b50fa3df876b63ce993c6b7f0eff1703eaeb

Moving the non-e10s reftests to Windows 10 seems to have helped (they were also retriggered a lot more in a previous try push). I think we're good to attempt a second landing.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1083fb72eed4
[reftest] Refactor manifest parsing from reftest.jsm to standalone module, r=jmaher

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1083fb72eed4