1189015 - After removing an animated element w/ Animations Devtools open, "ASSERTION: shouldn't have observed an animation being removed/added twice: 'Not Reached', file dom/base/nsDOMMutationObserver.h"

Status: NEW

(Core :: DOM: Animation, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load attached testcase in a debug build.
 2. Right-click the colorful box and choose "Inspect Element".
 3. Click "Animations" tab at right edge of devtools panel.
 4. Now, click the text in the testcase (which triggers the animated element to be removed). Or, remove the animated element manually by selecting it in the inspector & hitting "Del" key.

ACTUAL RESULTS:
At least one copy of this assertion:
 ###!!! ASSERTION: shouldn't have observed an animation being removed twice: 'Not Reached', file dom/base/nsDOMMutationObserver.h, line 830

...and usually also a variant of the same assertion, with s/removed/added/:

###!!! ASSERTION: shouldn't have observed an animation being added twice: 'Not Reached', file dom/base/nsDOMMutationObserver.h, line 790


...and usually many more copies of each of these assertions.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Birtles, mind taking a look?

Comment 3

[Brian Birtles (:birtles)]

Yeah, I've seen this before. Cameron might be better able to look at this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: An automation test to reproduce the assertions

The assertions are caused by when plural mutation observers observe the same element.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

It seems like a common use case. I think we should remove the assertions there.

Cameron, what do you think?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Remove assertions in nsAutoAnimationMutationBatch::AnimationAdded and AnimationRemoved

I did a comparative experiment on try server:

With assertions (current trunk):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef245b9d1bc

Without assertions:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05254621aec

It revealed that this assertion causes an intermittent failure (bug 1227792). I guess the reason of the failure is that obtaining stack traces for the assertion takes inadmissible time there.

As I wrote in comment #5, these assertions are hit in common use cases, we should remove them.

Comment 7

[Tim Nguyen :ntim]

:hiro, Thanks for taking a look at this !

Do you know if this fixes bug 1186675 as well ? Your try push says it did, but I just want to be sure.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Though I have no idea what kind of relationship between assertions and memory leaks, some of those failures might be fixed by attachment 8702749 [details] [diff] [review]. E.g.:
https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=6360621
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=19091107

But some of them won't:
https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=6360977
https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=6360664
There are no same assertions.

Comment 9

[Cameron McCormack (:heycam)]

Thanks Daniel for the test, and Hiro for investigating.  Sorry for dropping this for a while.

I think the assertions are valid and indicative of a bug.  In the test, when we remove the element from the document, we have a single nsAutoAnimationMutationBatch object on the stack, yet we call nsAutoAnimationMutationBatch::AnimationRemoved twice -- once for each of the two observers watching the element.  Maybe we should either (a) be ensuring that we only record the information about the animation addition/change/removal once, so that our state tracking is accurate, or (b) this state should be stored per observer (i.e. pass in the observer to those static Animation{Added,Changed,Removed} methods and use it as part of the key to look up mEntryTable).

I'll have a think about this; I'm not sure why DOM mutation observers don't suffer from this problem.

Comment 10

[Cameron McCormack (:heycam)]

nsAutoMutationBatch::NodesAdded calls Done every time, so it behaves a bit differently it seems.

I think the easiest thing to do is just ensure that the first time we pass on a given animation notification from the nsAnimationReceiver to the nsAutoAnimationMutationBatch that we update our state, and we don't update our state for subsequent calls to the batch for that same notification.  (Since there are multiple nsAnimationReceivers, we can't coordinate this "only inform the batch once" behaviour from nsAnimationReceiver, so instead let's do it from the code that invokes all of the nsIAnimationObservers.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8702749 [details] [diff] [review]
Remove assertions in nsAutoAnimationMutationBatch::AnimationAdded and AnimationRemoved

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

r=me on the test, but please leave the assertion.  I'll upload a patch to fix the underyling problem.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8702749 [details] [diff] [review]
Remove assertions in nsAutoAnimationMutationBatch::AnimationAdded and AnimationRemoved

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

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1489,2 @@
>  // Run the tests.
> +SimpleTest.expectAssertions(0);

I don't think this will do anything, since the default is 0.

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Process an animation mutation notification on a batch only once.

Comment 15

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

Comment on attachment 8705467 [details] [diff] [review]
Process an animation mutation notification on a batch only once.


>+++ b/dom/base/nsNodeUtils.cpp
>@@ -80,16 +80,17 @@ using mozilla::AutoJSContext;
>     nsDOMMutationObserver::EnterMutationHandling();               \
>   }                                                               \
>   nsINode* node = content_;                                       \
>   do {                                                            \
>     nsINode::nsSlots* slots = node->GetExistingSlots();           \
>     if (slots && !slots->mMutationObservers.IsEmpty()) {          \
>       /* No need to explicitly notify the first observer first    \
>          since that'll happen anyway. */                          \
>+      nsAutoAnimationMutationBatch::NextBatchNotification();      \

this looks weird.
Need to think about this a bit.

Comment 16

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

(In reply to Cameron McCormack (:heycam) from comment #10)
> nsAutoMutationBatch::NodesAdded calls Done every time, so it behaves a bit
> differently it seems.

So in case of DOM mutation ContentInserted/Appended, those method don't do really anything with the
mutation record. They just tell to the batch object that this particular mutationobserver is interested in the mutation.

ContentRemoved is a bit different, there the first call during batch ends up adding the removed node to the batch's removedNodes list.


 
> I think the easiest thing to do is just ensure that the first time we pass
> on a given animation notification from the nsAnimationReceiver to the
> nsAutoAnimationMutationBatch that we update our state, and we don't update
> our state for subsequent calls to the batch for that same notification. 
yes, this sounds right. 
But I don't understand how the patch helps with this. What if there a mutation observer is observing several ancestor nodes?
Doesn't your patch help only in case there are separate mutation observers observing ancestor nodes or something.

I think you could do something similar to ContentRemoved here. 
AnimationAdded/Changed/Removed would tell to batch that the particular observer is interested in that animation and
also record the animation, but only once.
Looks like the first part is there already (nsAutoAnimationMutationBatch::AddObserver call), but it is the
nsAutoAnimationMutationBatch::AnimationAdded/Changed/Removed which would need to check if something during the current batch already reported about that particular animation...
so doesn't that actually just mean the assertions are invalid.

Comment 19

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

Making DidComposite a high priority message, and thus letting RefreshDriver tick more often if normal priority queue is flooded seems to make this more often
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228117419&repo=mozilla-inbound&lineNumber=65478

Comment 20

[Cameron McCormack (:heycam)]

Won't have time to get back to this.