1240509 - Browser mochitest using focus() fails after previous test used EventUtils on Linux opt x64 e10s

Status: NEW

(Firefox :: General, defect)

Description

[Julian Descottes [:jdescottes]]

Attachment: focus-issue-testcase.diff

Follow up to Bug 1240368. Highlights intermittent test that depends on a previous test execution.

STRs

- pre : e10s only
- pre : Linux 64 opt only
- intermittent test : try on a slow VM, or slow it down manually to increase the chance of failures
- import focus-issue-testcase.diff
- run ./mach mochitest --e10s devtools/client/focusbug/test/browser_focusbug*
- (repeat a few times)

Actual : 

From time to time the second test will fail on with the following error message.

> The following tests failed: 17 INFO TEST-UNEXPECTED-FAIL | devtools/client
> /focusbug/test/browser_focusbug_02.js | The container is now focused. - Got 
> null, expected [object XULElement]

Expected : 

I assume nothing is wrong with the tests so : tests should always pass.
Unless anything _needs_ to be done to avoid the first test causing a crash of the second one.

===========================
Investigation : 

Running focusbug_02 independently never fails.

Some more details in Bug 1240368, but :

Waiting in the beginning of focusbug_02 fixes the issue. 

Triggering a "mousedown" event using EventUtils and waiting for the focus also fixes the issue (however I'm guessing that EventUtils is triggering events asynchronously so this might just work for the same reason as a simple "wait").

I tried checking if focus() might be asynchronous here, but trying to wait for a focus event to be raised after calling focus() never resolves.

Also while isolating the issue, I managed to have the same kind of failures in focusbug_02 when I simply had a js error in focusbug_01.

Finally, simply using a focus() in focusbug_01 has no impact on focusbug_02. Only had the issue when relying on EventUtils.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Neil, is this something you could look into?

Comment 2

[Neil Deakin]

The browser-test framework, between each test, removes the current tab and replaces it with a new blank tab. In the case of these tests, this happens to be redundant.

However, the tests here are relying on being able to insert a new element inside the area specific to the tab, which was just newly created. The issue occurs because the test is running before the new tab has been painted. This means that the visibility has not yet been updated, so the focus system believes the element 'container' to not be visible and thus cannot be focused.

The solution is to either:

1. Have the test framework wait for the new tab to be painted before running the next test
2. As a workaround, wait for a MozAfterPaint event to occur after appending the "container".

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Neil Deakin from comment #2)

> 2. As a workaround, wait for a MozAfterPaint event to occur after appending
> the "container".

Thanks for the info! This seems to fix the issue locally for me. Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf55f1491417 (applied to the initial tests that were failing in the devtools suite).

Comment 4

[Julian Descottes [:jdescottes]]

Neil :

Do we need to respect the order : 
- first : append the container
- second : wait for MozAfterPaint 

Would it be ok to simply wait for MozAfterPaint before starting the test ?

Comment 5

[Neil Deakin]

A paint should always occur after the container is appended, since there is something new to update/draw. If you wait for the paint before it might not happen as it likely happened before the test started.

Comment 6

[Brian Grinstead [:bgrins]]

It would be nice if we this fix could live somewhere in the test harness (maybe in testing/mochitest/browser-test.js?) rather than random tests needing it.  Not sure if that will cause issues in other tests/suites but might be worth a try push.

Although I notice that there's already mozafterpaint handling here: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#41.

Comment 7

[Neil Deakin]

That MozAfterPaint only handles the test harness loading. We want one after each test is run, after:

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#315

Comment 8

[Julian Descottes [:jdescottes]]

I had mixed results when waiting for MozAfterPaint after appending the container (cf https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87df8d2d5bd&selectedJob=15732578 )

It works in e10s mode, but introduces failures in non-e10s mode : MozAfterPaint is never fired and the test times out.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #8)
> I had mixed results when waiting for MozAfterPaint after appending the
> container (cf
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d87df8d2d5bd&selectedJob=15732578 )
> 
> It works in e10s mode, but introduces failures in non-e10s mode :
> MozAfterPaint is never fired and the test times out.

Maybe try putting it in the harness instead of individual tests, and possibly guarding it on `if (Services.appinfo.browserTabsRemoteAutostart)` to run it only on e10s if needed?

Comment 10

[Julian Descottes [:jdescottes]]

We ran two sets of try tests waiting for MozAfterPaint before each test (modifying browser-test.js as suggested).

- wait for MozAfterPaint in any mode : https://treeherder.mozilla.org/#/jobs?repo=try&revision=85633d235475
- wait for MozAfterPaint in e10s only : https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8e69a86692

There is a permanent failure on Linux 32bits debug non-e10s with the first one.
=> For now we should only wait for MozAfterPaint in e10s mode.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: bug1240509.wip.patch

Neil : This is the patch that ran on try, and fixes the issue. I'm not sure we're adding the event listener at the proper place though. You mentioned doing it around when we do addTab/removeTab. Is this when we are sure a MozAfterPaint will be triggered?

Comment 12

[Neil Deakin]

Comment on attachment 8711029 [details] [diff] [review]
bug1240509.wip.patch

This seems ok for now. I'll investigate why this is different for non-e10s.

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: bug1240509.v1.patch

Neil: would you be ok to review this? (same patch, just cleaned up a bit and changed the commit comment)

Let me know if you prefer it to be fwd to someone else.

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5b59fdd981a

Comment 14

[Neil Deakin]

Attachment: Wait for tab switch

We probably get paint events earlier than that. In non-e10s, perhaps even right after addTab. But I realized that a better approach is to just wait for the tab switch to be complete, which encompasses paint waiting, rather than specifically waiting just for painting.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5abb2d460b1

Comment 15

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07345a63101b

Comment 16

[Neil Deakin]

So I'm not sure from both those try builds whether this causes other intermittent oranges, since I'm not familiar with the devtools tests. Julian, could you take a look?

Comment 17

[Julian Descottes [:jdescottes]]

First of all, tree-abstract-01 and tree-abstract-02 ran in the same suite and didn't fail, which normally means the patch fixes the issue for this bug.

Now for the other DT* failure : 
- dt5 : we have had issues with the tests in /performance lately : the dt5 failures on Linux and Linux x64 are probably linked to this. Not related to your patch (looks like a fix is on its way in Bug 1204174)
- dt8 : I haven't seen them regularly lately, retriggered the job to see how frequent they are here.
- failures in Linux64opt dt4 and Win7debug dt2 didn't occur before. Both linked to dead CPOWs. Retriggered the jobs here as well.

Let's wait for the outcome of the restarted jobs.

Comment 18

[Neil Deakin]

So two different variations on this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5abb2d460b1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48969dd0985

have caused toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js to permanently fail in e10s linux. I can easily reproduce locally with the patch, but not without it. The patch causes the test harness to wait for the tab switch before starting the test.

Not sure why this test is dependent on that; maybe David as author of the test, can shed some light.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Is it the only test that fails?

This test depends indirectly on
- the amount of time since latest user input (but the other tests in the same directory have that same dependency, more or less);
- the idle service.

Could either of them be impacted by your change?

Comment 20

[Neil Deakin]

That's the only test that fails consistently. For the first try run linked above in comment 18, it fails always. For the second try run, when I changed the test to add and remove a tab at the beginning, it fails slightly less, but still often.

Normally the test harness opens a new blank tab in-between each test and the existing one closed. My change only changes the test harness to wait for the tab to be switched to before continuing. It likely makes the harness take longer between tests.

But the test shouldn't be relying on some state existing before the test runs, no?

Comment 21

[David Teller [:Yoric] - still alive but not very active]

No idea atm. For the moment, I'm a bit swamped., but I'll try and take a look at the end of the week.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Unfortunately, I didn't get the time to look at it today. Not sure when I will find that time.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

I don't think it's blocking webconsole tests anymore.
Not sure if this is even still valid ?