1180125 - Queue animation and transition events from Animation::Tick and dispatch as a separate step

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This is about implementing the following part from bug 1151731 comment 4:

> a) Make event queueing a separate step performed by Animation::Tick and dispatched in nsRefreshDriver::Tick

The reason this needs to be a separate step is that when we actually run these callbacks they should see a fully updated timing model so we need to make sure to update everything first *then* dispatch events.

I'd really like to reuse the work Xidorn did in bug 1168705 since it's very close to what we're doing here. However, there are two problems in doing that:

a) The nsRefreshDriver's PendingEvent setup expects an nsIDOMEvent. I'm not sure how expensive it is to turn all our internal events into nsIDOMEvents but even if we workaround that there's a bigger problem:

b) The way events are queued now is not in any particular order. We walk through the different collections in the linked list which is not sorted and then we add all the events for each element. As far as I can tell, there are no requirements on the ordering of events in the spec, but it seems like we should do something sensible: probably timeline order then document order.

With regards to (b), once we tick animations from their timelines (where they'll be stored in a hashmap) this ordering will be even more random so we'll probably want to introduce sorting to the events before dispatching them. That suggests we need to queue them (batch them might be a better word) then sort them just before dispatching. That's going to make it hard to reuse Xidorn's stuff unfortunately.

Comment 1

[Brian Birtles (:birtles)]

Attachment: WIP patch v1 - rework queueing and dispatch of CSS animation events

A very rough attempt at reworking the way CSS animation events are queued
and dispatched.

Applies on top of bug 1150810 (which needs bug 1171817) on central from
2015-07-02-ish (specifically bug 1171817 which just landed will conflict with
this).

TODO: Something similar to transitions
TODO: Rename 'queue' to 'batch'

Comment 2

[Brian Birtles (:birtles)]

Looking into a couple of test failures, I discovered this patch queues events with the pres context for its target element but:

  [pres context] != [pres context]->RefreshDriver()->PresContext()

so when we go to dispatch queued up events from the refresh driver we often don't find any to dispatch.

The easiest solution is probably to make nsAnimationManager::QueueEvent basically look for the root pres context and queue all events on its associated animation manager. I'm not sure if that's entirely valid, however.

It seems like:
* a refresh driver never updates it pres context until it is Disconnect-ed (which happens when the pres context is destroyed)
* likewise an animation manager never updates its pres context until it is Disconnected-ed (which happens when the pres context is destroyed)
* a pres context update its refresh driver:
  - when it is destroyed (set to null)
  - on init (set to 1 or 3 different possibilities including a new refresh driver)

So it probably is safe to just use the pres context associated with a refresh driver.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #2)
> Looking into a couple of test failures, I discovered this patch queues
> events with the pres context for its target element but:
> 
>   [pres context] != [pres context]->RefreshDriver()->PresContext()
> 
> so when we go to dispatch queued up events from the refresh driver we often
> don't find any to dispatch.

We can't traverse child documents to find the right pres context? Something similar to RunFrameRequestCallbacks[1] or GetProfileTimelineSubDocShells[2].

[1] https://hg.mozilla.org/mozilla-central/file/951116f384da/layout/base/nsRefreshDriver.cpp#l1493
[2] https://hg.mozilla.org/mozilla-central/file/951116f384da/layout/base/nsRefreshDriver.cpp#l1436

Comment 4

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> We can't traverse child documents to find the right pres context? Something
> similar to RunFrameRequestCallbacks[1] or GetProfileTimelineSubDocShells[2].

That's a good idea. I think we could either:

a) Always store events on the animation manager for the pres context associated with the refresh driver (and have calls to DispatchEvents defer this animation manager)
b) Store events on the appropriate animation manager and have the refresh driver iterate over all the descendent animation managers

(a) seems simpler but we might encounter problems if we try to sort events from different documents

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 1 - Queue and dispatch events as a separate step

Hi David, what do you think of this approach? Does it make sense? Let me know if
you'd prefer I split this patch up a bit more finely before asking you to look
at it.

If you agree with this approach, I plan to do the same thing for transitions.

Furthermore, I expect to add a further patch that sorts the events before
dispatching them since we'll need that before long (and it's probably good
to have a logical and consistent order for these anywhere).

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 0 - Fix an obvious bug in animation_utils.js

There was a bug introduced in to animation_utils.js in
https://hg.mozilla.org/mozilla-central/rev/496e867cd2cd (bug 1070745). This
patch fixes this bug so we can be sure testing events correctly before messing
with them.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 1 - Queue and dispatch CSS animation events as a separate step

This slightly simplifies the previous patch

Comment 8

[Brian Birtles (:birtles)]

For my own reference, I started working on removing the call to DispatchEvents in nsPresShell::FlushPendingNotifications because:

* There's nothing in the spec to say we should dispatch animationstart events in
  the same frame as an animation is generated. Rather, the only place this is
  really specified is HTML's processing model which says CSS animations and
  events are only done once per frame:
    https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-9
* When we make animationstart events not fire until the animation has actually
  started (bug 1134163), we won't be able to continue dispatching them in the
  same frame anyway.
* From an architectural point of view, flushing style is an animation function,
  but events are a timing function so we shouldn't be doing this when updating
  style.

However, without that call you have to wait *two* frames to get the animationstart event. Basically, what happens is:

Frame n:
- Update element style to create new animation
Frame n+1:
- Tick animations and queue events
- Dispatch events
- Process pending style changes
- Queue events <-- animationstart queued here
Frame n+2:
- Tick animations
- Dispatch events <-- animationstart dispatched here

If you trigger a style flush in frame n then you only have to wait one frame.

The call to call to DispatchEvents in nsPresShell::FlushPendingNotifications means that we dispatch the event in frame n+1 even if we don't trigger a style flush in frame n.

This is going to change in bug 1134163 anyway so I might just leave this alone for now.

Comment 9

[Brian Birtles (:birtles)]

Part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2d06619510

Comment 10

[Brian Birtles (:birtles)]

Attachment: WIP part 2 - Queue and dispatch CSS transition events as a separate step

This is a very rough first pass at this. I think I probably want to do
something like:

* Move CommonAnimationManager out of the css namespace (it's about time and it
  makes working on this code even more painful; one day I'll finally rename
  nsAnimationManager / nsTransitionManager to CSSAnimationManager
  / CSSTransitionManager and put them in the mozilla namespace too).
* Create a templated superclass for managing events that contains the QueueEvent
  / DispatchEvent etc. methods and make nsAnimationManager inherit from that
* Stick the method to convert a PseudoType to a string somewhere more sensible
* Make all the other changes bit by bit

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 1 - Queue and dispatch CSS animation events as a separate step

Just a minor tweak to reverse the order in which animation events and fullscreen (pending) events are dispatched to match the HTML spec

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 2 - Move AnimationCommon classes out of CSS namespace

The long-term plan is to drop the mozilla::css namespace altogether. Before we
go to much further with refactoring code in AnimationCommon, we should drop
usage of the mozilla::css namespace. Specifically, this patch moves the
CommonAnimationManager and AnimValuesStyleRule classes to the mozilla namespace.

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 3 - Extract DelayedEventDispatcher

This patch extracts a utility class for queueing up a series of EventInfo
objects (of templated type) and then dispatching them. This covers the event
queuing behavior in nsAnimationManager so that we can reuse it in
nsTransitionManager.

Comment 14

[Brian Birtles (:birtles)]

Attachment: part 4 - Move PseudoTypeAsString to AnimationCommon and reuse

Prior to this patch, CSSAnimation defined a method for converting an
nsCSSPseudoElements::Type to a nsString (but only for the set of
pseudo-elements that can have animations). We would like to re-use this
when setting up transition events so this patch moves it to
AnimationCollection. Re-using this method more widely means we can make
a few further simplifications to the code.

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 5 - Move TransitionEventInfo to nsTransitionManager.h

This is needed so we can allocate storage in nsTransitionManager for the
transition events we will queue for delayed dispatch.

Comment 16

[Brian Birtles (:birtles)]

Attachment: part 6 - Use DelayedEventDispatcher in nsTransitionManager

This simply uses the DelayedEventDispatcher in place of the previous array of
TransitionEventInfo objects. Doing the actual delayed dispatch is performed in
a separate patch.

Comment 17

[Brian Birtles (:birtles)]

Attachment: part 7 - Queue transition events from CSSTransition::Tick

This patch moves the logic for queueing events out of the logic for flushing
transitions making it a separate step. It still doesn't delay the dispatch of
those events into a separate step yet. That is done in a subsequent patch.

This patch also makes sure to clear any queued events when the nsPresShell that
owns the transition manager is destroyed. We don't expect CSSTransition::Tick to
be called anywhere except nsTransitionManger::FlushTransitions so there
shouldn't be any orphaned events but for completeness it seems best to add this
now. (Later, when we tick transitions from their timeline we will need this.)

Comment 18

[Brian Birtles (:birtles)]

Attachment: part 8 - Dispatch transition events from refresh driver

This patch causes transition events to be dispatched as a separate step after
sampling the transitions. Eventually this will allow us to sample transitions
from their timeline (independently of where they came from and in potentially
any order) by separating the concepts of sampling and event dispatch.

Comment 19

[Brian Birtles (:birtles)]

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

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633192 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

Is there a situation where we might queue events and then not run
the refresh driver tick shortly afterwards?  If so, it seems like
we might need to hook this up to the cycle collector.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633194 [details] [diff] [review]
part 3 - Extract DelayedEventDispatcher

>+  void DispatchEvents(nsPresContext* const & aPresContext)

No need for reference here; nsPresContext* const should be fine.


r=dbaron with that

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633196 [details] [diff] [review]
part 4 - Move PseudoTypeAsString to AnimationCommon and reuse

>Bug 1180125 part 4 - Move PseudoTypeAsString to AnimationCommon and reuse

AnimationCollection, not AnimationCommon.

>+/*static*/ nsString
>+AnimationCollection::PseudoTypeAsString(nsCSSPseudoElements::Type aPseudoType)
>+{
>+  switch (aPseudoType) {
>+    case nsCSSPseudoElements::ePseudo_before:
>+      return NS_LITERAL_STRING("::before");
>+    case nsCSSPseudoElements::ePseudo_after:
>+      return NS_LITERAL_STRING("::after");
>+    default:

Probably good to assert here that it's ePseudo_NotPseudoElement.

>+      return EmptyString();
>+  }
>+}

r=dbaron with that

Comment 23

[Brian Birtles (:birtles)]

Attachment: part 1b - Hook nsAnimationManager's list of events up to the cycle collector

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #20)
> Comment on attachment 8633192 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> Is there a situation where we might queue events and then not run
> the refresh driver tick shortly afterwards?  If so, it seems like
> we might need to hook this up to the cycle collector.

Yes, I think there could be such a situation. Even if it doesn't happen now, it
may well occur in the future so I think it's best to hook this up.

Here's a patch to do that. If this approach looks good to you, I'll apply the
same treatment to nsTransitionManager's list of events in a separate patch.

Comment 24

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #21)
> Comment on attachment 8633194 [details] [diff] [review]
> part 3 - Extract DelayedEventDispatcher
> 
> >+  void DispatchEvents(nsPresContext* const & aPresContext)
> 
> No need for reference here; nsPresContext* const should be fine.

The intention was that this parameter should continue to reflect the mPresContext member of the calling manager which I figured could go null if the code executed by EventDispatcher::Dispatch caused the prescontext to be destroyed (therefore triggering a call to CommonAnimationManager::Disconnect). Does that sound possible?

Comment 25

[Brian Birtles (:birtles)]

Attachment: part 3 - Extract DelayedEventDispatcher

Updated version of part 3 that moves the cycle-collection code to DelayedEventDispatcher

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #24)
> The intention was that this parameter should continue to reflect the
> mPresContext member of the calling manager which I figured could go null if
> the code executed by EventDispatcher::Dispatch caused the prescontext to be
> destroyed (therefore triggering a call to
> CommonAnimationManager::Disconnect). Does that sound possible?

Maybe.

So it's fine the way you had it *if* you add a comment explaining why.  Otherwise it's not clear.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8638366 [details] [diff] [review]
part 1b - Hook nsAnimationManager's list of events up to the cycle collector

>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Maybe good practice for this to be:
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIStyleRuleProcessor)

r=dbaron

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633192 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

So I guess one other concern about this patch:

we have logic to determine which documents share a refresh driver and which don't.  This seems to be overriding that logic by saying that if a refresh driver ticks, then all of its subdocuments will fire animation events right then, even if they're on a different refresh driver.  This seems at least odd, although I can't immediately describe harm that results from it.

Should this mechanism be more related to the way that refresh drivers currently relate to documents, or at least be structured not to descend into documents with different refresh drivers?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633199 [details] [diff] [review]
part 7 - Queue transition events from CSSTransition::Tick

The thing that seems a bit odd here is that it seems to be relying on side-effects in a rather unclear way in order to only fire the event once.  In particular, it's assuming that Tick() is going to be called exactly once between when GetComputedTiming() reports Computedtiming::AnimationPhase_After and when the transition's IsFinishedTransition() becomes true.  That seems rather sensitive.

Is there a better way to do things here?

Comment 30

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #28)
> Comment on attachment 8633192 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> So I guess one other concern about this patch:
> 
> we have logic to determine which documents share a refresh driver and which
> don't.  This seems to be overriding that logic by saying that if a refresh
> driver ticks, then all of its subdocuments will fire animation events right
> then, even if they're on a different refresh driver.  This seems at least
> odd, although I can't immediately describe harm that results from it.
> 
> Should this mechanism be more related to the way that refresh drivers
> currently relate to documents, or at least be structured not to descend into
> documents with different refresh drivers?

Yes, I think you're right. Events are queued during a call to:

1. nsAnimationManager::WillRefresh, i.e. when the manager is ticked by the refresh driver. In this case we will dispatch any queued events moments later within the same refresh driver tick.

2. PresShell::FlushPendingNotifications. In this same function we then call mPresContext->AnimationManager()->DispatchEvents() so any queued events will be dispatched immediately.

3. nsAnimationManager::CheckAnimationRule, which is called by nsStyleSet::GetContext. This case doesn't dispatch events immediately.

I had forgotten about 3. If it were only 1 and 2 it wouldn't matter if we descend into subdocuments on a different refresh driver since they wouldn't have any events queued.

Long-term bug 1134163 will probably remove (2), and I'd like to get this code to the point where we get exactly one call to Animation::Tick per refresh driver tick which would eliminate (3). Until then, we probably have to do the complex thing here. I'll update the patch with the refresh driver check.

Comment 31

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #29)
> Comment on attachment 8633199 [details] [diff] [review]
> part 7 - Queue transition events from CSSTransition::Tick
> 
> The thing that seems a bit odd here is that it seems to be relying on
> side-effects in a rather unclear way in order to only fire the event once. 
> In particular, it's assuming that Tick() is going to be called exactly once
> between when GetComputedTiming() reports
> Computedtiming::AnimationPhase_After and when the transition's
> IsFinishedTransition() becomes true.  That seems rather sensitive.
> 
> Is there a better way to do things here?

Yes, you're right. That's much less safe than I realized. I overlooked it because I already fixed it in bug 1181392 (attachment 8638412 [details] [diff] [review]). I'll bring that patch forward to this bug.

Comment 32

[Brian Birtles (:birtles)]

Attachment: part 1 - Queue and dispatch CSS animation events as a separate step

Comment 33

[Brian Birtles (:birtles)]

Attachment: part 7 - Queue transition events from CSSTransition::Tick

Comment 34

[Brian Birtles (:birtles)]

With only 5 jobs left to run, this may be my best try for 2015:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57be14991588

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639132 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

>+  virtual ~nsAnimationManager() {}

I don't see why you need this.  nsAnimationManager is final.

r=dbaron on the rest

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639133 [details] [diff] [review]
part 7 - Queue transition events from CSSTransition::Tick

>transitions making it a separate step. It still delay the dispatch of

Did you mean to leave "doesn't delay" as this was in the previous commit message?

>(mPreviousPhaseOrItertaion) which we may be able to unify at the same point.

Itertaion -> Iteration (I hope!)

>   virtual ~nsTransitionManager() {}

Hmmm.  This also shouldn't be here -- but it was introduced in something else that happened between the old patch and this one.


r=dbaron

Comment 37

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> Comment on attachment 8639132 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> >+  virtual ~nsAnimationManager() {}
> 
> I don't see why you need this.  nsAnimationManager is final.

Without this I was seeing the following warning on OSX/Android/B2G:

   error: static_assert failed "Reference-counted class nsAnimationManager should not have a public destructor. Make this class's destructor non-public"

Comment 38

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #37)
> Without this I was seeing the following warning on OSX/Android/B2G:
> 
>    error: static_assert failed "Reference-counted class nsAnimationManager
> should not have a public destructor. Make this class's destructor non-public"

I should add that this only became necessary when I specifically added the cycle-collection methods to nsAnimationManager (and later, nsTransitionManager). Both of these classes were ref-counted before that (via CommonAnimationManager) but the MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING wasn't being applied to them then.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb39baea4559
https://hg.mozilla.org/integration/mozilla-inbound/rev/79bcd4f744c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d45871d805
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb4e4b1c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5996d0e410a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/587b33b52ee0
https://hg.mozilla.org/integration/mozilla-inbound/rev/99d1cfef2722
https://hg.mozilla.org/integration/mozilla-inbound/rev/658e49586449
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad38421023a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/677efdac9819

Comment 40

[Carsten Book [:Tomcat]]

sorry had to back this out for since one of this changes might have caused bug 1188799 at least started with this pushes

Comment 41

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85842339a74e
https://hg.mozilla.org/integration/mozilla-inbound/rev/379289054053
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbecd9b966f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2e5b611ab9
https://hg.mozilla.org/integration/mozilla-inbound/rev/299e06ab9606
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabf79f37ff8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d219e6f0a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe818bba6c2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a5d306eb92
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c3a66ff523

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Bug 1188799 confirmed fixed by this backout.

Comment 43

[Brian Birtles (:birtles)]

I'm having trouble working out what is going on with the failing browser_translation_exceptions.js test. There is something subtly different about the timing (or perhaps order) of dispatching transitionend events that means that sometimes when we get to the transitionend handler registered in translation-infobar.xml's init method this._getAnonElt is undefined where as othertimes it is filled in.

I don't know enough about XBL to know why this would happen and it's proving hard to debug. I can't reliably reproduce it and changing almost anything in translation-infobar.xml causes it to stop failing. (I guess this would be a good place for rr if I had a linux box.)

So far as I can tell, it's not the case that we're getting transitionend events for a different element although that could be the case if there's a nested <navigation> element firing events (again, I can't reproduce this once I add instrumentation to compare the event target with the <navigation> element).

I suspect if I replaced waiting for transitionend events with getting the animations running on the navigation element and waiting on their finished promise we might be able to fix this but first I need to dig into exactly why we're expecting a transitionend event.

Comment 44

[Brian Birtles (:birtles)]

Ok, the transitions we're waiting on are here:
https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/toolkit/content/xul.css#l200

I've confirmed that we're getting two animation objects on the transition element for each of the transition properties (in both failure and success cases).

Beyond that I'm not sure what's happening.

* I don't *think* it's a problem that we have two transitions and, for example, we're getting processing one transitionend event after the other and having destroyed 'this'. Firstly, I don't think there's anything different with regards to the granularity with which we batch and dispatch events within the same element, and secondly I've never seen anything in the small amount of logging I've succeeded in doing during the fail case to indicate we're processing a second transitionend event.

* I don't *think* it's a problem that 'this' is not in scope during the transitionend event or points to a different element. I've succeeded in confirming that 'this' has the same identity when registering the event listener and inside the event callback.

* I don't *think* it's a problem that the order of the opacity and margin-left events is random. Firstly, it shouldn't be (these patches should preserve ordering) and secondly I've confirmed we dispatch them in the same order in both failure and success cases.

* I wondered if it was to do with having a nested element firing transitionend events that the parent was picking up and that something had changed with regards to the order in which we dispatch transitionend events between ancestors and descendents, particularly if, due to our use of XBL here, we were crossing subdocument boundaries. However, I have at least confirmed that in both success and failure cases the element target is a <navigation> element.

However, I think the last one might be close. I've just observed that in the failure case the event target's parent node is null, whilst in the success case it is navigationbox.

What could be happening is we have some other element listening for transitionend events in another subdocument and the handler for that event is doing some document surgery.

Before this bug (specifically part 8) we'd run FlushTransitions on all the TransitionManagers attached to the ticking refresh driver and dispatch events right away. Now, however, we'll queue them all then dispatch them beginning with the document of the pres context associated with the refresh driver and then visiting all the subdocuments. I suppose that will mean that we might process outer events before inner events. So if there is some ancestor document that does cleanup or tree surgery on transitionend then when we get to the transitionend for the inner <navigation> element it might have already been torn down.

Comment 45

[Brian Birtles (:birtles)]

I think I might have finally narrowed this down a bit.

In browser_translation_exceptions.js we get to the check "check shouldShowInfoBar initially returns true" then yield waiting for the pop-up to open.

In the success case, we dispatch transitionend events (opacity and margin-top) while waiting and pass them to the infobar and notification (infobar extends notification).

In the failure case, however, we resume the test (i.e. the Promise from openPopup resolves) before the transitionend events get dispatched. Then we proceed with the test to the point where we close the infobar.

Next we go to re-open the infobar and yield waiting for the pop-up. At this point the initial transitionend event is delivered but by this point the original notification is closed (and partially destroyed?) and we run into trouble.


Digging into this a bit further, the Promise-yielding openPopup() method we use to wait with is based on listening for the "popupshown" event from the notif._getAnonElt("options") element which is a <xul:button>.

So it sounds like there's a race between the popupshown event firing on the <xul:button> in the anonymous content of the infobar and the transitionend event firing on the parent <navigation>. We've come across this issue before in bug 994117 where we added code to nsMenuPopupFrame.cpp to delay the popupshown event until transitions had fired (https://hg.mozilla.org/mozilla-central/rev/0e9d11e5302a).

I notice this code is disabled on GTK which may be why bug 1188799 only fails on Ubuntu (there is one failure on OSX but it's a timeout that's incorrectly filed).

If that's correct, then the patches in this bug are probably just exposing an existing race (if I change just about anything in the test, the failure disappears so it's very time sensitive). Alternatively (and perhaps more likely), with these patches introduced, we may be ticking something else in between queueing transition events and dispatching them that causes the popupshown event to get in ahead of the transitionend event.

In either case, it would seem like simply enabling the code from bug 994117 on GTK as well would fix this.

Neil or Neil, do you know why the block of code in nsMenuPopupFrame::SetInitialChildList in https://hg.mozilla.org/mozilla-central/rev/0e9d11e5302a is disabled on GTK?

Comment 46

[[e-mail]]

Bug 1001234 and related bugs are probably relevant here.

Comment 47

[Neil Deakin]

I think you may be confusing the transition on the infobar with transitions on a panel. Transitions are disabled on panels on Linux due to graphical issues, so popup transitions don't come into play here. Also, it's a menu on a button which doesn't have a transition anyway (only arrow panels currently have them).

The test should wait for the infobar transition to finish before continuing I'm guessing, otherwise the button with the menu being opened isn't fully visible when it tries to open it.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #37)
> (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> > Comment on attachment 8639132 [details] [diff] [review]
> > part 1 - Queue and dispatch CSS animation events as a separate step
> > 
> > >+  virtual ~nsAnimationManager() {}
> > 
> > I don't see why you need this.  nsAnimationManager is final.
> 
> Without this I was seeing the following warning on OSX/Android/B2G:
> 
>    error: static_assert failed "Reference-counted class nsAnimationManager
> should not have a public destructor. Make this class's destructor non-public"

Does it work if you make it private and skip the "virtual"?

Comment 49

[Brian Birtles (:birtles)]

(In reply to Neil Deakin from comment #47)
> I think you may be confusing the transition on the infobar with transitions
> on a panel. Transitions are disabled on panels on Linux due to graphical
> issues, so popup transitions don't come into play here. Also, it's a menu on
> a button which doesn't have a transition anyway (only arrow panels currently
> have them).

The disabled code in question appears to be affecting more than it was intended to. We are seeing transitions on Linux for this infobar but we're following a codepath in Gecko that assumes we don't have transitions on Linux. How is that not a bug?

> The test should wait for the infobar transition to finish before continuing
> I'm guessing, otherwise the button with the menu being opened isn't fully
> visible when it tries to open it.

I thought the purpose of bug 994117 was that tests don't have to wait for *both* transitionend and popupshown to fire but could rely on popupshown happening later? That's how it works on all platforms except Linux. I'm happy to change that test but that doesn't seem to fix the root problem and it probably won't be long before someone else wastes 2 days debugging an "intermittent only on Linux" problem.

Comment 50

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #48)
> (In reply to Brian Birtles (:birtles) from comment #37)
> > (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> > > Comment on attachment 8639132 [details] [diff] [review]
> > > part 1 - Queue and dispatch CSS animation events as a separate step
> > > 
> > > >+  virtual ~nsAnimationManager() {}
> > > 
> > > I don't see why you need this.  nsAnimationManager is final.
> > 
> > Without this I was seeing the following warning on OSX/Android/B2G:
> > 
> >    error: static_assert failed "Reference-counted class nsAnimationManager
> > should not have a public destructor. Make this class's destructor non-public"
> 
> Does it work if you make it private and skip the "virtual"?

I expect so (this assertion doesn't run on Windows so I can't quickly check) but what's the issue with protected and explicit "virtual"? (The dtor is already virtual due to CommonAnimationManager.)

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #50)
> I expect so (this assertion doesn't run on Windows so I can't quickly check)
> but what's the issue with protected and explicit "virtual"? (The dtor is
> already virtual due to CommonAnimationManager.)

Oh, I missed the virtual destructor in CommonAnimationManager.  Probably fine then.  (Although maybe CommonAnimationManager doesn't need a virtual destructor anymore?)

Comment 52

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #51)
> Oh, I missed the virtual destructor in CommonAnimationManager.  Probably
> fine then.  (Although maybe CommonAnimationManager doesn't need a virtual
> destructor anymore?)

That seems risky since if we add anything to ~nsAnimationManager it might not get called when deleting from a pointer to the base class.

Comment 53

[Neil Deakin]

(In reply to Brian Birtles (:birtles) from comment #49)
> The disabled code in question appears to be affecting more than it was
> intended to. We are seeing transitions on Linux for this infobar but we're
> following a codepath in Gecko that assumes we don't have transitions on
> Linux. How is that not a bug?

I think you might still be confusing the infobar with panels. The infobar isn't a panel and doesn't fire a popupshown event. It has a transition and fires the transitionend event when it is finished, which the test does not wait for.

The test then opens a menu popup from a button that is a child of the infobar, The popup does not itself have a transition but does fire the popupshown event when it is finished opening.

On Linux there shouldn't be any transition on any menus or panels. Your patch just changes the popup code to check if there is one, but there shouldn't be one.

I haven't tested this myself though, so it's possible I'm not understanding something about this specific notification being tested. If someone has added a transition to this specific menu, it shouldn't be there on Linux.

Comment 54

[Brian Birtles (:birtles)]

(In reply to Neil Deakin from comment #53)
> I think you might still be confusing the infobar with panels. The infobar
> isn't a panel and doesn't fire a popupshown event. It has a transition and
> fires the transitionend event when it is finished, which the test does not
> wait for.
> 
> The test then opens a menu popup from a button that is a child of the
> infobar, The popup does not itself have a transition but does fire the
> popupshown event when it is finished opening.
> 
> On Linux there shouldn't be any transition on any menus or panels. Your
> patch just changes the popup code to check if there is one, but there
> shouldn't be one.

Thanks for that, that's a helpful explanation. The transition I'm seeing is this one:

  https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/toolkit/content/xul.css#l200

which appears to apply to the translationbar in this case.

At the same time, the menu popup's animate attribute is set to open and the menu popup's frame has current transitions on it.

I don't know how the XBL mapping works here but perhaps the code in nsMenuPopupFrame is picking up the transition on the infobar causing this to pass on non-Linux platforms, when it really shouldn't. That might be because we're now looking up whether we have animations by frame, instead of by content (https://hg.mozilla.org/mozilla-central/rev/2fd98d3d9258), I'm not sure.

It sounds like it's ok to change the test anyway.

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94a07b627f21
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30ecbdba384
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c94690a852b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd9620537f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9549506b9cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/50892e26a6ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/e901b139e679
https://hg.mozilla.org/integration/mozilla-inbound/rev/0162e758928d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a55501a48f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7054bcee797

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94a07b627f21
https://hg.mozilla.org/mozilla-central/rev/f30ecbdba384
https://hg.mozilla.org/mozilla-central/rev/7c94690a852b
https://hg.mozilla.org/mozilla-central/rev/6bd9620537f0
https://hg.mozilla.org/mozilla-central/rev/d9549506b9cf
https://hg.mozilla.org/mozilla-central/rev/50892e26a6ec
https://hg.mozilla.org/mozilla-central/rev/e901b139e679
https://hg.mozilla.org/mozilla-central/rev/0162e758928d
https://hg.mozilla.org/mozilla-central/rev/c9a55501a48f
https://hg.mozilla.org/mozilla-central/rev/a7054bcee797