1022852 - Fix test failures with Linux BasicCompositor OMTC enabled

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

After applying the patches attached to the blocking bugs on bug 994541, enabling the Linux compositor causes some browser-element OOP tests to fail.

https://tbpl.mozilla.org/?tree=Try&rev=72522d622e02

Comment 1

[Chris Lord [:cwiiis]]

Note, the failing tests pass on my local machine (consistently, I can't get them to fail), so not really sure what to do with this yet...

Comment 2

[Chris Lord [:cwiiis]]

Reading through the bug that added this test, I'm not really any closer to a theory on why it would fail (and only on tbpl...)

Yuan, do you have any ideas here? I'm pretty stuck.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, other than the failures covered by bug 1020162, which other tests are failing?  I can try locally on my machine, see if I can reproduce.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Is the test expecting a particular order of messages that is not guaranteed?

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Chris, other than the failures covered by bug 1020162, which other tests are
> failing?  I can try locally on my machine, see if I can reproduce.

These are the only tests that are failing, except they are now consistently failing rather than intermittently failing. I'm not sure about ordering, perhaps it's an issue of not waiting long enough(?) My desktop machine is likely quite fast compared to the test machines.

Comment 6

[Karl Tomlinson (:karlt)]

Is the time the test already takes to run close to the limit?

Did the test pass with compositor without the patches from other bugs?

Perhaps choas mode might help in reproducing.
http://robert.ocallahan.org/2014/03/introducing-chaos-mode.html

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

In the test of browserElement_setInputMethodActive.js, we create one input frame and two im (input method) frames. The async message from the input frame is sent after that from the im frame, but is received before that from the im frame sometimes. The unexpected message ordering causes this bug.

@timdream, do you have resource to make a fix?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I would love to take this but before commit to anything I would like to know the expectation first. If you are asking a fix for this week then I will not be able to deliver. I can probably schedule to do this next week.

To clarify, can you point out the right order to assert here if it's not 1->2->3->4->5->6?

http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElement_SetInputMethodActive.js#163

Is it something like (1&2) -> (3&4) -> 5 -> 6?

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Maybe (1 & 2) -> (3 & 4 & 5) -> 6 since there are calls to setInputMethodActive() on (2) and (5)?

And how do I make sure the patch is correct?

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP (untested)

Without waiting for discussion to continue and try to reproduce this on my machine, here is my untested patch. If this works we should take this.

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8440540 [details] [diff] [review]
WIP (untested)

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

::: dom/browser-element/mochitest/browserElement_SetInputMethodActive.js
@@ +189,5 @@
> +
> +          break;
> +      }
> +
> +      if (gFrameMsgCounts.input !== 1 || gFrameMsgCounts.im0 !== 1) {

We should ensure gFrameMsgCounts.input <= 1. If we get gFrameMsgCounts.input == 2, the test fails.

@@ +224,2 @@
>  
> +      if (gFrameMsgCounts.input !== 2 ||

Similar to above.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Tim, thanks. The logic is really complex. I hope we can find a way to simplify it.

Comment 13

[Chris Lord [:cwiiis]]

The attached patch didn't seem to work:

https://tbpl.mozilla.org/?tree=Try&rev=f3e438dada90

Note, I had to apply it manually due to the Windows line-endings.

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8440540 [details] [diff] [review]
WIP (untested)

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

Applying the patch results in timeouts and a different failure: https://tbpl.mozilla.org/?tree=Try&rev=f3e438dada90

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP (worked locally)

Hi Chris,

This is the updated patch. I can verify this works locally with m-c today and I have rebased it against bug 1020726.

I tried to push a try by apply your try commits, however there is a conflict so I was unable to do that. Would you mind rebase your patch and push everything to try again? Thanks.

Comment 16

[Chris Lord [:cwiiis]]

So it seems at some point, someone's introduced a pref that disables omtc specifically on tests? Seems like a bad idea to me... But I guess this failure no longer blocks turning on Linux OMTC in that case(?)

Try push with just Linux OMTC enabled: https://tbpl.mozilla.org/?tree=Try&rev=e5a2c8a1bb92
Try push with above + this patch: https://tbpl.mozilla.org/?tree=Try&rev=f2bc968e1653
Try push with above + OMTC enabled in tests: https://tbpl.mozilla.org/?tree=Try&rev=deb5c4337a19

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 8445715 [details] [diff] [review]
WIP (worked locally)

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

Looks promising, good work! Can you seek review so we can get this landed soon?

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for commit

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee3c7e56ab3

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ee3c7e56ab3