1427987 - Fix devtools/server/tests/browser/browser_navigateEvents.js in e10s

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

This test currently fails with a timeout:
TEST-START | devtools/server/tests/browser/browser_navigateEvents.js
Adding a new tab with URL: http://test1.example.org/browser/devtools/server/tests/browser/navigate-first.html
Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
TEST-PASS | devtools/server/tests/browser/browser_navigateEvents.js | Get first page load - 
TEST-PASS | devtools/server/tests/browser/browser_navigateEvents.js | undefined assertion name - 
Tab added and URL http://test1.example.org/browser/devtools/server/tests/browser/navigate-first.html loaded
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_navigateEvents.js | Test timed out -

Comment 1

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1427987 - Fix browser_navigateEvents.js on e10s.

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

Comment 2

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8940661 [details]
Bug 1427987 - Fix browser_navigateEvents.js on e10s.

This is not handy, win32 builds are broken on try, so I can't confirm non-e10s is green on try, but it is for me locally...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff3a93875b6dc9b05269a5b3605fdec39a046d06&selectedJob=154700049

I'll try to rebase later and repush to try before merging.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8940661 [details]
Bug 1427987 - Fix browser_navigateEvents.js on e10s.

https://reviewboard.mozilla.org/r/210904/#review217076

::: devtools/server/tests/browser/browser_navigateEvents.js:15
(Diff revision 1)
> +var onFinished;
> +var finished = new Promise(done => {
> +  onFinished = done;
> +});

It took me some time to understand the logic and I think it's only due to how these variables are named. It would make it easier to understand like this I think:

```
var signalAllEventsReceived;
var onAllEventsReceived = new Promise(resolve => {
  signalAllEventsReceived = resolve();
});
```

::: devtools/server/tests/browser/browser_navigateEvents.js:69
(Diff revision 1)
>        is(event, "tabNavigated", "Finally, the receive the client event");
>        is(data.state, "stop", "state is stop");
>        is(data.url, URL2, "url property is correct");
>        is(data.nativeConsoleAPI, true, "nativeConsoleAPI is correct");
>  
> -      // End of test!
> +      onFinished();

This way, it's more self-explanatory to call `signalAllEventsReceived()` here. It reads easier.

::: devtools/server/tests/browser/browser_navigateEvents.js:82
(Diff revision 1)
> +    setTimeout(() => {
>        let stack = browser.parentNode;
>        let dialogs = stack.getElementsByTagName("tabmodalprompt");
>        let {button0, button1} = dialogs[0].ui;
>        callback(button0, button1);
> -    });
> +    }, 1000);

Why 1 second here? This seems arbitrary and pretty long. This used to be an `executeSoon` which I guess is closer to a `setImmediate`, right? What is the reason for waiting that long here? And is there a reason this could create race conditions later?

::: devtools/server/tests/browser/browser_navigateEvents.js:171
(Diff revision 1)
> -function cleanup() {
> -  let browser = gBrowser.selectedBrowser;
> -  browser.removeEventListener("DOMContentLoaded", onDOMContentLoaded);
> -  browser.removeEventListener("load", onLoad);
> -  client.close().then(function () {
> +  // Load another document in this doc to dispatch these events
> +  assertEvent("load-new-document");
> +  BrowserTestUtils.loadURI(browser, URL2);
> +
> +  // Wait for all events to be received
> +  await finished;

And it's equally easier to read this line like this:

`await onAllEventsReceived;`

Comment 4

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8940661 [details]
Bug 1427987 - Fix browser_navigateEvents.js on e10s.

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

Comment 5

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Patrick Brosset <:pbro> from comment #3)
> ::: devtools/server/tests/browser/browser_navigateEvents.js:82
> (Diff revision 1)
> > +    setTimeout(() => {
> >        let stack = browser.parentNode;
> >        let dialogs = stack.getElementsByTagName("tabmodalprompt");
> >        let {button0, button1} = dialogs[0].ui;
> >        callback(button0, button1);
> > -    });
> > +    }, 1000);
> 
> Why 1 second here? This seems arbitrary and pretty long. This used to be an
> `executeSoon` which I guess is closer to a `setImmediate`, right? What is
> the reason for waiting that long here? And is there a reason this could
> create race conditions later?

This code was copy pasted from another browser test:
  https://searchfox.org/mozilla-central/source/toolkit/components/startup/tests/browser/head.js#22-27
Which evolved since it has been copied.
But with e10s enabled, using waitUtil for focused window doesn't work, nor executeSoon.
So I ended up using this:
  waitUntil(() => dialogs[0]);
This prevents the precise exception we are trying to avoid.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8940661 [details]
Bug 1427987 - Fix browser_navigateEvents.js on e10s.

https://reviewboard.mozilla.org/r/210904/#review217492

Comment 7

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 1 changes to 1 files (+1 heads)
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://treestatus.mozilla-releng.net/trees/try :
remote: HTTP Error 503: Service Unavailable
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b49df8dc9b9
Fix browser_navigateEvents.js on e10s. r=pbro

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9b49df8dc9b9