1104908 - Re-enable the Style Inspector tests on Linux e10s when they meet visibility standards

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

We have multiple bugs on file for various oranges in this suite right now that aren't getting attention. The failure rate is pushing 20% based on the retriggers I've been doing and the majority of the failures are within styleinspector. I'm disabling them for now.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f2d2962cc060

Comment 2

[Chris Peterson [:cpeterson]]

e10s test

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[Patrick Brosset <:pbro>]

The first test I looked at is browser/devtools/styleinspector/test/browser_computedview_original-source-link.js
The failure for this one is pretty obvious, it relies on a 5sec max timeout to wait for something to happen instead of waiting on specific events. It probably fails intermittently when the test machine gets really slow.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1104908-1-styleinspector-e10s v1.patch

Added a couple of events to the style-inspector to allow the test to listen to them instead of using a timeout.

Comment 6

[Patrick Brosset <:pbro>]

The next set of tests I'm going to take a look at are the colorpicker related tests. There's a bunch of them that have been intermittently failing because whenever they try to simulate a color change (using simulateColorPickerChange in head.js), they're actually using waitForSuccess to wait for the color to be applied to the element, and that also waits for 5sec maximum. So, same story as in comment 4.

Comment 7

[Patrick Brosset <:pbro>]

One way to check that a changed done in the style-inspector is actually applied to the page would be to use the StyleRuleChanged chrome-only event. The problem with that is it doesn't seem to be dispatched to content docShells. I haven't found any way of listening to this event with e10s, from a frame script. And events don't bubble across process boundaries, so I can't listen to the event from the mochitest either.

NI? bz for help on a way to make it possible to listen for style rule changes in mochitest with e10s, Paul suggested you might know of a way.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1104908-1-browser_computedview_original-source-link.js v2.patch

Revised the previous patch with just a quick change to avoid the "TypeError: this._elementStyle.rules is undefined" errors we've also been seeing in tests.

Comment 9

[Patrick Brosset <:pbro>]

I just checked a few other tests that fail intermittently, the reasons pretty much all come down to waitForSuccess not waiting for long enough. So we definitely need to move away from it and rely on events instead.

Comment 10

[Heather Arthur [:harth]]

Comment on attachment 8535011 [details] [diff] [review]
bug1104908-1-browser_computedview_original-source-link.js v2.patch

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

awesome, thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

I have no idea about listening to chromeonly events in e10s, sorry.  Olli might...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> One way to check that a changed done in the style-inspector is actually
> applied to the page would be to use the StyleRuleChanged chrome-only event.
> The problem with that is it doesn't seem to be dispatched to content
> docShells. I haven't found any way of listening to this event with e10s,
> from a frame script. And events don't bubble across process boundaries, so I
> can't listen to the event from the mochitest either.
Add a listener to the TabChildGlobal?

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Olli Pettay [:smaug] from comment #12)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> > One way to check that a changed done in the style-inspector is actually
> > applied to the page would be to use the StyleRuleChanged chrome-only event.
> > The problem with that is it doesn't seem to be dispatched to content
> > docShells. I haven't found any way of listening to this event with e10s,
> > from a frame script. And events don't bubble across process boundaries, so I
> > can't listen to the event from the mochitest either.
> Add a listener to the TabChildGlobal?

This, unfortunately, doesn't seem to be working. I've tried all I could think of to listen to that event, but couldn't make it to work.

Anyway, my other approach is to remove the 5s timeout altogether in waitForSuccess (in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/head.js#514), it's too unstable to wait for something to change for 5s on try, and in any case, the test harness will detect a timeout if the value doesn't change to what we expect.

I pushed to try with the patch to fix browser_computedview_original-source-link.js and a patch that removes the timeout: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5fab0cdd5353
It seems that now, the only remaining failing test is browser_styleinspector_tooltip-closes-on-new-selection.js.
It fails a lot with e10s unfortunately, but it does seem to be the only remaining test, so I'll take a look at this one next.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8535011 [details] [diff] [review]
bug1104908-1-browser_computedview_original-source-link.js v2.patch

Pushed the first patch to fx-team: https://hg.mozilla.org/integration/fx-team/rev/483622be4d4b

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1104908-2-removed-waitforsuccess-timeout v1.patch

Heather, what about removing the timeout in waitforsuccess altogether, and letting the test harness handle timeouts if really the change we're expecting in the test doesn't occur?

As reported earlier, this function alone is responsible for most of the e10s styleinspector test failures, and there really isn't a good way to listen to a kind of "css rule has changed on the element/page" event (I've tried using StyleRuleChanged as said above).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5fab0cdd5353 shows that all the tests using this function seem to be quite stable now (the one test that still fails in this try push isn't related to this function).

Comment 16

[Heather Arthur [:harth]]

Comment on attachment 8536526 [details] [diff] [review]
bug1104908-2-removed-waitforsuccess-timeout v1.patch

If it helps then that's fine. The nice thing about having that timeout is you get a message with the name of the thing that timed out, which makes finding a bug a bit easier. But I'd be fine with this.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/483622be4d4b

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bug1104908-2-removed-waitforsuccess-timeout v2.patch

Modified the commit message.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug1104908-3-renenable-styleinspector-e10s-tests v1.patch

This final patch (part 3) is the one that actually re-enables the test on linux+e10s.
So, it enables all but 2 tests that have been frequently failing and that I think would be better handled in another bug: bug 1111546.
These 2 are XUL panel related and might have some implications outside of devtools. That's why I decided to file another bug.

Here's a new try push with all 3 patches in: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5a9c7f02e82

Comment 20

[Patrick Brosset <:pbro>]

That try push looks pretty good, I'm pushing the 2 last patches to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/8b769fd1123e
https://hg.mozilla.org/integration/fx-team/rev/b746b1589448

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8537137 [details] [diff] [review]
bug1104908-3-renenable-styleinspector-e10s-tests v1.patch

r=me this patch since this is just the changes to browser.ini to enable the test on e10s and disable the 2 tests I mentioned in a previous comment.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b769fd1123e
https://hg.mozilla.org/mozilla-central/rev/b746b1589448