1115276 - browser/browser/components/places/tests/browser/browser_555547.js fails intermittently when animations don't start promptly

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brian Birtles (:birtles)]

In bug 927349 we are making CSS animations and transitions start when they have rendered their first frame. Unfortunately this seems to cause browser_555547.js to intermittently fail on Windows XP opt.

It seems like this test is doing interaction that depends on animations starting more promptly.

I notice that this test has failed intermittently for others:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/899016

For now I'm skipping this test for Windows opt builds but we should investigate how to rewrite this test to be more robust.

Comment 1

[Marco Bonardo [:mak]]

we show the toolbar and open the context menu for the first item, I'm not sure if the problem is with the click or with the context menu, we could definitely wait for the context menu to open and see if that helps.

Comment 2

[Brian Birtles (:birtles)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> we show the toolbar and open the context menu for the first item, I'm not
> sure if the problem is with the click or with the context menu, we could
> definitely wait for the context menu to open and see if that helps.

I tried adding a executeSoon or rAF wait after opening the context menu and it didn't seem to help:

executeSoon:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d27b86f54af1

rAF:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabc52e6fbf9

Maybe I should try before the click?

Comment 3

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #2)
> Maybe I should try before the click?

Trying that now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f2997265e2

Comment 4

[Marco Bonardo [:mak]]

ugh, right, we do animate the bookmarks toolbar when we change its visibility, basically here

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_555547.js#15

Any test using setToolbarVisibility on the bookmarks toolbar is potentially affected... we need a test util to toggle visibility on the bookmarks toolbar and wait for the animation to be complete.

Comment 5

[Marco Bonardo [:mak]]

browser_drag_bookmarks_on_toolbar.js implements some code we can probably reuse in the function afterToolbarTransition, we could merge setToolbarVisibility and that into an helper and use that in both places

Comment 6

[Brian Birtles (:birtles)]

Attachment: Make tests that set toolbar visibility wait for any animations to finish before continuing

Here's an initial attempt at implementing some of the suggestions on comment
4 and comment 5.

Let me know what you think.

Currently running through try:

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

Note that the minimal change from comment 3 (simply adding a rAF callback in
bug_555547.js) also fixes this. This is just an attempt to do something more
generic as suggested.

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8547962 [details] [diff] [review]
Make tests that set toolbar visibility wait for any animations to finish before continuing

>+/**
>+ * Waits for a clipboard operation to complete, looking for the expected type.
>+ *
>+ * @see waitForClipboard
>+ *
>+ * @param aPopulateClipboardFn
>+ *        Function to populate the clipboard.
>+ * @param aFlavor
>+ *        Data flavor to expect.
>+ */
>+function promiseClipboard(aPopulateClipboardFn, aFlavor) {
>+  let deferred = Promise.defer();
>+  waitForClipboard(function (aData) !!aData,
>+                   aPopulateClipboardFn,
>+                   function () { deferred.resolve(); },
>+                   aFlavor);
>+  return deferred.promise;
>+}

Oops, I don't know how that crept in there. Ignore that part, I've removed it locally.

Comment 8

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #6)
> Created attachment 8547962 [details] [diff] [review]
> Make tests that set toolbar visibility wait for any animations to finish
> before continuing
> 
> Here's an initial attempt at implementing some of the suggestions on comment
> 4 and comment 5.
> 
> Let me know what you think.
> 
> Currently running through try:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd4955055883

Hmm, looks like this didn't fix things.

It works for me locally so I'm not sure what's happening on try but I'd guess we're not getting the transitionend event like we expect. Perhaps we need to set up the listener before tweaking the visibility.

Comment 9

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #8)
> It works for me locally so I'm not sure what's happening on try but I'd
> guess we're not getting the transitionend event like we expect. Perhaps we
> need to set up the listener before tweaking the visibility.

Trying that now, just in case (but I don't expect it should make a difference):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=368dabed6170

Also, with some extra debugging (but I think dump() doesn't work in opt build so this might not help?):

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

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8547962 [details] [diff] [review]
Make tests that set toolbar visibility wait for any animations to finish before continuing

Cancelling review until I work out what is going on with try.

Comment 11

[Brian Birtles (:birtles)]

Attachment: Patch v2

Comment 12

[Brian Birtles (:birtles)]

Minimal try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79983d024476

In progress, cross-platform try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d3920dc6492

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8548701 [details] [diff] [review]
Patch v2

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

I think we can get a slightly smaller patch here

::: browser/base/content/browser.js
@@ +4912,5 @@
> +function _getToolbarHidingAttribute(toolbar) {
> +  return toolbar.getAttribute("type") == "menubar" ? "autohide" : "collapsed";
> +}
> +
> +function isToolbarVisible(toolbar) {

The problem is that "autohide" = "true", doesn't really tell whether the toolbar is visible or not, so this may be lying afaict.

Until we really need this util in browser.js I'd suggest doing whatever you need just in head.js, it's too easy to introduce tempting not-100%-correct utils in browser, have add-ons rely on them, and then figure later we didn't want them.
duplicating the getAttribute code there is not a big deal, it's a oneliner

For now I'd completely leave browser.js unchanged.

@@ +4913,5 @@
> +  return toolbar.getAttribute("type") == "menubar" ? "autohide" : "collapsed";
> +}
> +
> +function isToolbarVisible(toolbar) {
> +  let hidingAttribute = _getToolbarHidingAttribute(toolbar).toLowerCase();

why toLowerCase() this, when the result can already only be lowercase?

::: browser/components/places/tests/browser/browser_475045.js
@@ +7,5 @@
>                       getService(Ci.mozIJSSubScriptLoader);
>  this._scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", ChromeUtils);
>  
>  function test() {
> +  waitForExplicitFinish();

you could change test to an add_task and use the promise version, it would require far less changes and should not break most of the blame info

::: browser/components/places/tests/browser/browser_555547.js
@@ +7,5 @@
>  
>    let sidebarBox = document.getElementById("sidebar-box");
>    is(sidebarBox.hidden, true, "The sidebar should be hidden");
>  
> +  let checkSidebar = function() {

ditto, using the promise version would preserve blame

::: browser/components/places/tests/browser/head.js
@@ +418,5 @@
> + *
> + * @param aToolbar
> + *        The toolbar to update.
> + * @param aVisibility
> + *        True to make the toolbar visible, false to make it hidden.

To avoid confusion with "visibility", I'd just call this aVisible "Whether the toolbar should be visible."

@@ +423,5 @@
> + * @param aCallback
> + *        An optional callback to be called when the toolbar has finished
> + *        animating.
> + */
> +function waitForSetToolbarVisibility(aToolbar, aVisibility, aCallback) {

You don't need a callback version, just make a promise version, and if you cannot yield use .then(your_cb)

@@ +441,5 @@
> +      // Just because max-height is a transitionable property doesn't mean
> +      // a transition will be triggered, but it's more likely.
> +      aToolbar.addEventListener("transitionend", listener);
> +    } else {
> +      executeSoon(aCallback);

with just the promise version you won't need to executeSoon (since promises are resolved on the next tick already)

@@ +466,5 @@
> + *           finished.
> + * @rejects Never.
> + */
> +function promiseSetToolbarVisibility(aToolbar, aVisibility, aCallback) {
> +  let deferred = Promise.defer();

do not use .defer() in new code, it's deprecated.
instead use the return new Promise((resolve, reject) => { ... }) construct (or in very rare cases PromiseUtils.defer())

Comment 14

[Brian Birtles (:birtles)]

Attachment: Make tests that set toolbar visibility wait for any animations to finish before continuing

Comment 15

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #14)
> Created attachment 8553375 [details] [diff] [review]
> Make tests that set toolbar visibility wait for any animations to finish
> before continuing

Try run of the same:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc4c8cca482c

Comment 16

[Brian Birtles (:birtles)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13)
> The problem is that "autohide" = "true", doesn't really tell whether the
> toolbar is visible or not, so this may be lying afaict.
> 
> Until we really need this util in browser.js I'd suggest doing whatever you
> need just in head.js, it's too easy to introduce tempting not-100%-correct
> utils in browser, have add-ons rely on them, and then figure later we didn't
> want them.

Oh, I didn't realise add-ons could use this code. In that case, yeah, we should definitely drop this.

> @@ +4913,5 @@
> > +  return toolbar.getAttribute("type") == "menubar" ? "autohide" : "collapsed";
> > +}
> > +
> > +function isToolbarVisible(toolbar) {
> > +  let hidingAttribute = _getToolbarHidingAttribute(toolbar).toLowerCase();
> 
> why toLowerCase() this, when the result can already only be lowercase?

Is that always going to be true? If other code starts touching those attributes (e.g. setting "TRUE") then I think this method would sometimes produce the wrong result and we'd end up with an even more intermittent version of this bug? Seems better to avoid that?

All other changes made as suggested.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8553375 [details] [diff] [review]
Make tests that set toolbar visibility wait for any animations to finish before continuing

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

::: browser/components/places/tests/browser/head.js
@@ +397,5 @@
> +      // Just because max-height is a transitionable property doesn't mean
> +      // a transition will be triggered, but it's more likely.
> +      aToolbar.addEventListener("transitionend", listener);
> +    } else {
> +      resolve();

looks like in this case you would resolve before invoking setToolbarVisibility.

It's going to work regardless because the promise is resolved on the next tick, but I'd prefer to explicit that in code to make it more robust against future changes.

I'd be fine repeating setToolbarVisibility in both branches (you could also early return in the if and avoid the else

  ...
  aToolbar.addEventListener("transitionend", listener);
  setToolbarVisibility(aToolbar, aVisible);
  return;
}

// There's no animation to wait for.
setToolbarVisibility(aToolbar, aVisible);
resolve();

Comment 18

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03194db8d4f5

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03194db8d4f5

Comment 21

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e7224465ff5

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for mochitest-bc failures. Brian, these failures are hitting Aurora too. Can you please help rebase and get this re-landed? :)
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd6e773ba8ed

https://treeherder.mozilla.org/logviewer.html#?job_id=565050&repo=mozilla-aurora

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

It gets better, comment 27 is actually pointing to new bustage we're going to hit when Gecko 38 uplifts to Aurora in a couple weeks. I'll file a new bug and marking it blocking this one.

Comment 24

[Brian Birtles (:birtles)]

Do we need this on Aurora? The failures for bug 1118308 on aurora seem reasonably infrequent.

Also, which "comment 27" is comment 23 referring to? I'm afraid I might not be able to look at this for a while (PTO for the rest of this week, meetings all next week).

Comment 25

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Sorry, that should have been comment 22. Anyway, the "Do we need this on Aurora?" question is moot given bug 1129697. We can either address these failures now or in two weeks when 38 merges to Aurora. Your call.

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4c5fd24109f

Comment 27

[Brian Birtles (:birtles)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #25)
> Sorry, that should have been comment 22. Anyway, the "Do we need this on
> Aurora?" question is moot given bug 1129697. We can either address these
> failures now or in two weeks when 38 merges to Aurora. Your call.

Gotcha. Yes, that makes sense. Thanks for doing this!

Comment 28

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

All's well that ends well :)