Closed Bug 1094252 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
e10s + ---
firefox40 --- fixed

People

(Reporter: Gijs, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Points: --- → 3
e10s test bugs block e10s, but not a particular milestone.
tracking-e10s: --- → +
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.
Blocks: 921935
No longer blocks: 921935
Depends on: 921935
The test browser_locationBarExternalLoad.js is similar.
Summary: e10s - fix browser_locationBarCommand.js to work on e10s → e10s - fix browser_locationBarCommand.js and browser_locationBarExternalLoad.js to work on e10s
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attached patch browser_locationbartests (obsolete) — Splinter Review
This patch works but I am investigating whether I can fix bug 917535 as well.
Iteration: --- → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 39.2 - 23 Mar
Attachment #8580146 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8580146 - Flags: review?(gijskruitbosch+bugs) → feedback+
> 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.
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Attached patch browser_locationbartests (obsolete) — Splinter Review
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.
Attachment #8580146 - Attachment is obsolete: true
Attachment #8582767 - Flags: review?(gijskruitbosch+bugs)
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 ?
Attachment #8582767 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attached patch Rework testSplinter Review
Hopefully, one last time.
Attachment #8582767 - Attachment is obsolete: true
Attachment #8584441 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8584441 - Flags: review?(gijskruitbosch+bugs) → review+
I checked this in again using the TabSwitchDone event instead:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cb4ecfdcf4b1
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
https://hg.mozilla.org/mozilla-central/rev/cb4ecfdcf4b1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.