1329045 - Rewrite mochitest test_use_with_hsts.html to use async/await

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

The mochitest test_use_with_hsts.html does some loads inside of an iframe, and as a result, it involves some callback hell.  (It's written as a chain of callbacks.)

Now that we have async/await (in Firefox 52 according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#Browser_compatibility ), we can rewrite this code to be more direct & avoid the ugly callback chaining.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

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

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html.

(Before we introduced async/await in this test, some of these needed to be
global, because they were used in several callbacks.)

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

Comment 3

[Daniel Holbert [:dholbert]]

So this bug is basically condensing code patterns like the following:
>  function() Step1() {
>   iframe.onload = Step2;
>   iframe.src = someURI;
>  }
>  function() Step2() {
>   // inspect iframe
>   // ...
>  }

...to this:
>  await LoadIframeAsync(someURI);
>  // inspect iframe  

...using my provided LoadIframeAsync helper and the magic of Async/Await.  One note: some online sources recomment wrapping "await" expressions in try{}catch{}, since await will throw if the promise is rejected.  I haven't bothered with that here, since:
 (a) the only promise in this test (the one returned by LoadIframeAsync) is never rejected under any conditions -- it's only resolved.
 (b) if it happens to be rejected, then an exception is fine -- it'll just trigger a test failure, which is what we want anyway. (If this were common then we could add try/catch for more graceful reporting, but I'm not expecting it to be possible in the first place.)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102794/#review103248

::: dom/svg/test/test_use_with_hsts.html:70
(Diff revision 1)
>  
> -  // TEST CODE BEGINS HERE.
> -  // Execution basically proceeds top-to-bottom through the functions
> -  // from this point on, via a chain of iframe onload-callbacks.
> -  function runTest() {
> +  // Convenience helper which makes |iframe| load the given |uri|. Returns
> +  // a promise that resolves when the load completes. This makes it handy to
> +  // use with 'await', to avoid onload callback hell.
> +  async function LoadIframeAsync(uri) {
> +    return new Promise(function(resolve, reject) {

Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all.

::: dom/svg/test/test_use_with_hsts.html:72
(Diff revision 1)
> +      iframe.onload = function() {
> +        iframe.onload = null; // (only invoke this callback for this one load)
> +        resolve();
> +      };

You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler.

::: dom/svg/test/test_use_with_hsts.html:110
(Diff revision 1)
> -    // Point iframe at insecure (HTTP) version of testcase (which should get
> -    // automatically upgraded to secure (HTTPS) under the hood), & wait for
> +    // Load insecure (http) version of testcase:
> +    await LoadIframeAsync(insecureURI);

The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8824269 [details]
Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102796/#review103254

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

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

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8824269 [details]
Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html.

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

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

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

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8824269 [details]
Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html.

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

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102794/#review103248

> Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all.

Done -- that does work & makes this more compact. Thanks!

> You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler.

Indeed! Thanks, fixed.

> The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed.

Good point -- I've added it back. (I also reverted a few other needless wording tweaks, too.)

Comment 11

[Daniel Holbert [:dholbert]]

(Note: my "v2" of these patches (comment 6) included some bonus commit-message cruft left over from "qfold".  I noticed that just after pushing, & cleaned it up in "v3".)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa001860e4ab
part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/1a3de277a1fc
part 2: Convert some global variables to local variables, in test_use_with_hsts.html. r=xidorn

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa001860e4ab
https://hg.mozilla.org/mozilla-central/rev/1a3de277a1fc