Closed Bug 1104908 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Inspector, defect)

x86
Linux
defect
Not set
normal

Tracking

(e10s+)

RESOLVED FIXED
Firefox 37
Tracking Status
e10s + ---

People

(Reporter: RyanVM, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Blocks: dte10s
e10s test
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Added a couple of events to the style-inspector to allow the test to listen to them instead of using a timeout.
Attachment #8534951 - Flags: review?(fayearthur)
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.
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8534951 - Attachment is obsolete: true
Attachment #8534951 - Flags: review?(fayearthur)
Attachment #8535011 - Flags: review?(fayearthur)
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 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!
Attachment #8535011 - Flags: review?(fayearthur) → review+
I have no idea about listening to chromeonly events in e10s, sorry.  Olli might...
Flags: needinfo?(bzbarsky)
(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?
(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 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
Attachment #8535011 - Flags: checkin+
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).
Attachment #8536526 - Flags: review?(fayearthur)
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.
Attachment #8536526 - Flags: review?(fayearthur) → review+
Blocks: 1111546
Modified the commit message.
Attachment #8536526 - Attachment is obsolete: true
Attachment #8537134 - Flags: review+
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
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 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.
Attachment #8537137 - Flags: review+
Attachment #8537137 - Flags: checkin+
Attachment #8537134 - Flags: checkin+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8b769fd1123e
https://hg.mozilla.org/mozilla-central/rev/b746b1589448
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.