1094252 - e10s - fix browser_locationBarCommand.js and browser_locationBarExternalLoad.js to work on e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (out for now; he/him)]

Fails with various focus checks not having the correct result:

16 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js | There should be no focused element - Got [object XULElement], expected null
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:828
chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js:runShiftLeftClickTest/listener</</<:41
chrome://mochikit/content/browser-test.js:testScope/test_executeSoon/<.run:858
null:null:0
17 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js | Content window should be focused - Got [object ChromeWindow], expected [object CPOW [object Window]]
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:828
chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js:runShiftLeftClickTest/listener</</<:42
chrome://mochikit/content/browser-test.js:testScope/test_executeSoon/<.run:858
null:null:0


ad infinitum.

Comment 1

[Chris Peterson [:cpeterson]]

e10s test bugs block e10s, but not a particular milestone.

Comment 2

[Neil Deakin]

This test retrieves gFocusManager.focusedElement and gFocusManager.focusedWindow at several points. It should instead either have the child send a message containing the focused element at the right time, or just check contentDocumentAsCPOW.activeElement.

The window can be verified by comparing document.activeElement and gBrowser.selectedBrowser.

The test should also wait for the focus to actually happen when needed. Currently, it is using the 'activate' event when it could just use waitForFocus or at least the focus event.

Comment 3

[Neil Deakin]

The test browser_locationBarExternalLoad.js is similar.

Comment 4

[Neil Deakin]

Attachment: browser_locationbartests

This patch works but I am investigating whether I can fix bug 917535 as well.

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8580146 [details] [diff] [review]
browser_locationbartests

Review of attachment 8580146 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +333,5 @@
>  skip-if = e10s # Bug 921957 - remote webprogress doesn't supply cancel method on the request object
>  [browser_keywordSearch_postData.js]
>  [browser_lastAccessedTab.js]
>  skip-if = toolkit == "windows" # Disabled on Windows due to frequent failures (bug 969405)
>  [browser_locationBarCommand.js]

Are you confident this will also address the linux intermittents?

::: browser/base/content/test/general/browser_locationBarCommand.js
@@ +12,5 @@
> +  // Monkey patch saveURL to avoid dealing with file save code paths
> +
> +  // Alt and left click to save the url
> +  yield new Promise((resolve, reject) => {
> +    var oldSaveURL = saveURL;

Nit: let

@@ +31,5 @@
> +    Services.obs.addObserver(function observer(aSubject, aTopic) {
> +      newwindow = aSubject;
> +      Services.obs.removeObserver(observer, aTopic);
> +      executeSoon(() => resolve());
> +    }, "browser-delayed-startup-finished", false);

I think it would be better to reuse an existing "wait for a window to completely load" promise listener for this.

@@ +54,3 @@
>  
> +  let test = gTests.shift();
> +  while (test) {

do {
  let test = gTests.shift();
...

while (gTests.length);

@@ +74,5 @@
> +    while (gBrowser.tabs.length > 1) {
> +      gBrowser.removeTab(gBrowser.selectedTab);
> +    }
> +
> +    test = gTests.shift();

and nuke this, please. :-)

@@ +152,5 @@
>      EventUtils.synthesizeKey("VK_RETURN", aEvent);
>  }
>  
>  /* Checks that the URL was loaded in the current tab */
>  function checkCurrent(aTab) {

This is a generator function that should have a * if we're using those...

@@ +162,5 @@
>    is(gBrowser.selectedTab, aTab, "New URL was loaded in the current tab");
>  }
>  
>  /* Checks that the URL was loaded in a new focused tab */
>  function checkNewTab(aTab) {

Likewise.

@@ +172,5 @@
>    isnot(gBrowser.selectedTab, aTab, "New URL was loaded in a new tab");
>  }
>  
> +function* promisePageShow(browser, expectedURL) {
> +  yield new Promise((resolve, reject) => {

This should just return the promise and not be a generator function, AFAICT.

@@ +184,4 @@
>    });
>  }
>  
> +function* promiseCheckChildFocusedElement(browser)

This returns something and never yields, so it isn't a generator function, right?

Also, this seems to essentially check whether focusedElement === null, so maybe we should call it "promiseCheckChildNoFocusedElement"?

Also, can we make it return a boolean instead of a string?

Comment 6

[Neil Deakin]

> Are you confident this will also address the linux intermittents?

No, I intended to remove that again. But I am getting closer to finding out the cause.

> > +    Services.obs.addObserver(function observer(aSubject, aTopic) {
> > +      newwindow = aSubject;
> > +      Services.obs.removeObserver(observer, aTopic);
> > +      executeSoon(() => resolve());
> > +    }, "browser-delayed-startup-finished", false);
> 
> I think it would be better to reuse an existing "wait for a window to
> completely load" promise listener for this.

I need something that waits for a window to open, but I don't have access to the window yet.

Comment 7

[Neil Deakin]

Attachment: browser_locationbartests

I addressed everything except 'reuse an existing "wait for a window to completely load"' since I couldn't find an existing function that looked suitable.

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8582767 [details] [diff] [review]
browser_locationbartests

Review of attachment 8582767 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I have questions below. Up to you if there's a lot to change or not (and whether you want a re-review based on that)

::: browser/base/content/test/general/browser_locationBarCommand.js
@@ +72,5 @@
> +      if (gMultiProcessBrowser && test.check == checkNewTab) {
> +        // Trigger the command and wait for the new tab to be focused.
> +        // In multi-process mode, the browser will get focused, blurred, then
> +        // focused again in _adjustFocusAfterTabSwitch, so wait for two focus
> +        // events on the browser.

Isn't this a bug?

@@ +89,5 @@
>  
> +      triggerCommand(test.click, test.event);
> +    });
> +
> +    yield test.check(tab);

Shouldn't this be yield* because the function you're calling has its own yields ?

Comment 9

[Neil Deakin]

It turns out that Tim changed this test significantly in bug 1100291 so I'm redoing this work.

> ::: browser/base/content/test/general/browser_locationBarCommand.js
> @@ +72,5 @@
> > +      if (gMultiProcessBrowser && test.check == checkNewTab) {
> > +        // Trigger the command and wait for the new tab to be focused.
> > +        // In multi-process mode, the browser will get focused, blurred, then
> > +        // focused again in _adjustFocusAfterTabSwitch, so wait for two focus
> > +        // events on the browser.
> 
> Isn't this a bug?

Very likely. Bug 1108555 caused this to blur the focused element only to later focus it again. I'd have to look at that bug in more detail to understand what it was actually trying to accomplish.

Comment 10

[Neil Deakin]

Attachment: Rework test

Hopefully, one last time.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8584441 [details] [diff] [review]
Rework test

Review of attachment 8584441 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_locationBarCommand.js
@@ +191,5 @@
>    });
>  }
>  
>  function promiseNewTabSelected() {
> +  // In mutli-process mode, we need to wait for the focus event that comes

Nit: multi-process

@@ +241,5 @@
> +{
> +  if (!gMultiProcessBrowser) {
> +    return Services.focus.focusedElement == null;
> +  }
> + 

Nit: trailing whitespace.

Comment 12

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6df235c83a4a

Comment 13

[Neil Deakin]

I checked this in again using the TabSwitchDone event instead:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cb4ecfdcf4b1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb4ecfdcf4b1