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

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

unspecified
Firefox 38
x86_64
Windows 8.1
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
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.
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.
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
See Also: → 1118308
Assignee

Comment 2

5 years ago
(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?
Assignee

Comment 3

5 years ago
(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
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.
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
Assignee

Comment 6

5 years ago
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.
Attachment #8547962 - Flags: review?(mak77)
Assignee

Updated

5 years ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee

Comment 7

5 years ago
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.
Assignee

Comment 8

5 years ago
(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.
Assignee

Comment 9

5 years ago
(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
Assignee

Comment 10

5 years ago
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.
Attachment #8547962 - Flags: review?(mak77)
Assignee

Comment 11

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8547962 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8548701 - Flags: review?(mak77)
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())
Attachment #8548701 - Flags: review?(mak77)
Assignee

Updated

5 years ago
Attachment #8548701 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
(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
Assignee

Comment 16

5 years ago
(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 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();
Attachment #8553375 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/03194db8d4f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.1 - 26 Jan
Depends on: 1126633
Assignee

Updated

4 years ago
Duplicate of this bug: 1118308
Flags: needinfo?(ryanvm)
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
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles)
Depends on: 1129697
Assignee

Comment 24

4 years ago
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).
Flags: needinfo?(ryanvm)
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.
Flags: needinfo?(ryanvm)
Assignee

Comment 27

4 years ago
(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!
All's well that ends well :)
You need to log in before you can comment on or make changes to this bug.