Closed Bug 1247243 Opened 9 years ago Closed 9 years ago

Animations are shown only every 2 reloads

Categories

(DevTools :: Inspector: Animations, defect, P1)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: pbro, Assigned: nchevobbe)

References

Details

Attachments

(2 files, 3 obsolete files)

animation.html
2.94 KB, text/html
Details
Bug_1247243.patch
22.03 KB, patch
nchevobbe
: review+
Details | Diff | Splinter Review
Attached file animation.html — 
STR:

- open the attached HTML test case
- open the inspector and animation panel
- reload the page multiple times

-> expected: the animations below the <body> node (which is the one selected by default) are shown every time you reload the page

-> actual: this only works every other reload.
When the animation remains empty, the following error stacks are logged:

"Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: to is null
Stack: DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1617:11
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
DevToolsUtils.executeSoon*executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:34:11
LocalDebuggerTransport.prototype.send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:562:7
Front<.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1162:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
AnimationsController.destroyAnimationPlayers<@chrome://devtools/content/animationinspector/animation-controller.js:388:13
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
AnimationsController.refreshAnimationPlayers<@chrome://devtools/content/animationinspector/animation-controller.js:331:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
AnimationsController.onNewNodeFront<@chrome://devtools/content/animationinspector/animation-controller.js:244:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
Selection.prototype.setNodeFront@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/selection.js:185:5
InspectorPanel.prototype.onNewRoot/onNodeSelected@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:409:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.WalkerFront<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3407:12
InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:261:16
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.WalkerFront<.getMutations<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3533:12
exports.WalkerFront<.onMutations<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3663:5
Front<.onPacket/results<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1201:44
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1201:23
DebuggerClient.prototype.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:957:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
DevToolsUtils.executeSoon*executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:34:11
LocalDebuggerTransport.prototype.send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:562:7
DSC_send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1391:5
ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:742:5
Line: 1617, column: 11"

Looks like something's wrong with how we destroy animation actors on reload.
Blocks: 1201278
As you suggested it, I would be glad to work on this. Can assign it to me ?
Assignee: nobody → chevobbe.nicolas
So, I can reproduce it, but it is kind of random.
One thing I notice when it break is that animation scrubber is still moving while the inspector panel is refreshed.
I have trouble going deeper though, as I don't have as much log messages as you, although I activated preferences mentioned here https://wiki.mozilla.org/DevTools/Hacking . What am I missing here ?
Flags: needinfo?(pbrosset)
I don't have any other special prefs turned ON, the error log just appears in the browser console (ctrl+shift+J) whenever it failed to reload the animations.
Now, of course, trying again today and I can't reproduce :( I have no idea what's happening though. Yesterday I could get it to fail every time, and now it just works prefectly all the time (FF46 and 47).

If you manage to reproduce, that's a good start. Given the stack trace I pasted in comment 0, maybe you can use that to set a breakpoint at the right place in the destroy part and see what happens there?
Flags: needinfo?(pbrosset)
I thing I have an other STR, that cause an error during animationController.shutdown  :

- open the attached HTML test case
- open the inspector and animation panel
- open the browser console
- with focus on the browser console, keyboard reload ( Cmd + R )

-> actual : 

Error: Connection closed, pending request to server1.conn2.child1/animationsActor23, type stopAnimationPlayerUpdates failed

Request stack:
Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
AnimationsController.destroyAnimationPlayers<@chrome://devtools/content/animationinspector/animation-controller.js:390:13
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
AnimationsController.destroy<@chrome://devtools/content/animationinspector/animation-controller.js:186:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
shutdown<@chrome://devtools/content/animationinspector/animation-controller.js:55:9
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
destroy@chrome://devtools/content/animationinspector/animation-controller.js:68:10
ToolSidebar.prototype.destroy<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:541:15
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
InspectorPanel.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:605:28
Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1996:26
DT_forgetBrowserWindow@resource://devtools/client/framework/gDevTools.jsm:1274:9
gBrowserInit.onUnload@chrome://browser/content/browser.js:1402:5
onunload@chrome://browser/content/browser.xul:1:1

Sometimes I only get message like :

    Protocol error (noSuchActor): No such actor for ID: server1.conn0.child1/animationplayer49

And the tab kind of crash : the devtools panel shuts down and the tab redirect/show about:blank
I'm not sure that it's the same problem that you filed here, but It seems related.

In the terminal logs, I can see this :

EMITTING: emit(send, [object Object]) from LocalDebuggerTransport.prototype.send() -> resource://devtools/shared/transport/transport.js:548
DBG-SERVER: Packet 66 sent from "server1.conn2.child1/animationplayer49"

and shortly after :

DBG-SERVER: Received packet 66: {
  "from": "server1.conn2.child1/animationplayer37",
  "error": "noSuchActor",
  "message": "No such actor for ID: server1.conn2.child1/animationplayer37"
}

So it looks like a player emit something and is destroyed before the "listeners" can handle the message emitted.

I had a look at stopAnimationPlayerUpdates to see what can cause the error

  stopAnimationPlayerUpdates: method(function() {
    if (this.observer && !Cu.isDeadWrapper(this.observer)) {
      this.observer.disconnect();
    }
  }, {
    request: {},
    response: {}
  })

Ok, so it check if we have an observer ( which is a MutationObserver ), and if we do, stop the observation. The code looks pretty simple and I don't really know what's going on.

So that's where I'm here for now, I'll try to dig deeper later. Can you tell me if you can reproduce with the steps described here ?
Thank you
Flags: needinfo?(pbrosset)
Attached patch Bug_1247243_v_01.patch (obsolete) — — Splinter Review 
Please, ignore Comment 4 as it don't seems to be related.

So my guess on this is the following :

When the page is reloaded, animation-controller.js destroy function is not called, but there are "new-node-front" events which are emitted ( with "null" when reload, and another one when the markup is shown in the inspector panel )

Because we have 

  gInspector.selection.on("new-node-front", this.onNewFront )

the onNewFront function can be called twice in a short time span, even if the first call is not resolved. onNewFront calls the destroyAnimationPlayers function.
So it may occurs that the second destroyAnimationPlayers call tries to release players that where already released during the first call : 

  for (let front of this.animationPlayers) {
    yield front.release()    
  }

I guess we need to make sure to call the onNewFront function only after other pending calls to it are over.

I'm *really* not sure of what I'm submitting here. It adds a new function, which handle a queue for the onNewFront calls. The call to the function is done only where the calls in the queues are done.

Feel free to tell me if it is confusing, or if there is already some way to handle such things in the sources.

Thank you
Attachment #8719169 - Flags: feedback?(pbrosset)
Thanks for the investigation and patch. This turns out to be a much harder problem than I originally thought.
You're right though, the problem occurs when we're trying to navigate and when we receive new-node-front events.

I'm not sure however that queuing is the right solution for this. Imagine the case where you'd keep the down arrow key pressed for a long time in the markup-view. This would have the effect of selecting new nodes very rapidly in the inspector and therefore dispatching many rapid new-node-front events to the animation controller.
If we use a queue, then we'd run everything to completion for each and every node, slowly building up a delay that would be increasing more and more.
In a case like this, what the user really wants to see is the final node, they just happened to have been navigating to that final node by selecting each and every node in between.

So we rather need a way to cancel an update, rather than do it and queue the next ones.

Where it gets complicated is that to solve this, I think we shouldn't be managing the AnimationPlayerActors lifetime cycles on the client-side.
Right now, the animation-controller (so, the client-side) is responsible for releasing these actors when it knows it doesn't need them anymore. But it's also responsible for calling stopAnimationPlayerUpdates, which is also related to lifetime management.

Rather than doing this, I think the correct way of going forward is to not call release, nor stopAnimationPlayerUpdates in destroyAnimationPlayers, and make that function synchronous (remove Task.async).
In fact, we should remove everything that deals with releasing actors in animation-controller.js.

Now, with this removed, we still need to make sure those actors do go away at some stage, otherwise we'll end up with a big memory leak.

So, on the server (in animation.js), we need to make sure stopAnimationPlayerUpdates gets called when needed. It used to be called everytime destroyAnimationPlayers would be called on the client. So, on destroy of the panel, and on new-node-front.
Turns out that on the server, in AnimationsActor, this function is already called on destroy (which will be called when the panel is destroyed), and during getAnimationPlayersForNode which gets called everytime a new node is selected (so, on new-node-front). 
So, nothing to do here.

We also need to make sure all AnimationPlayerActor instances go away when needed. They used to be released when a "removed" mutation was received, on destroy of the panel, and on new-node-front.
Turns out there's a nice property to the client-server protocol we're using. There's a parent-child relationship between actors where a parent actor dictates the lifetime of a child actor:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.js.md#460
And AnimationPlayerActor is a child of AnimationsActor, so without doing anything, whenever the AnimationsActor goes away, then all children will be destroyed too (so, released).
This will happen when the panel is destroyed, which means that for as long as it is used, we'll just be adding more children and never releasing them, up until the end when we'll release them all at the same time.
At the very least, we could change getAnimationPlayersForNode so that instead of just doing `this.actors = [];` (where, conveniently, this.actors is the list of children actors we created so far), we could also do:
this.actors.forEach(actor => actor.destroy());
And remove the comment that says that lifetimes is handled on the client-side.

That's all I can think for now. I do think this is the right approach and that it will solve the weird issues we've been seeing in this bug. We'll need to carefully test that all expected actors get destroyed when we think they should be destroyed.
Flags: needinfo?(pbrosset)
Attachment #8719169 - Flags: feedback?(pbrosset)
I implemented what you suggests and I can reproduce the bug anymore.

> We'll need to carefully test that all expected actors get destroyed when we think they should be destroyed.

So I guess, on the server side, add tests :

- open a page and the animation inspector, select a node, check the number of players, select a node, check the number of player again ( we should not have the previous players )
- open a page and the animation inspector, select a node, check the number of players, delete the animation on it, check the number of player again ( we should not have any players )

Am I right, or should we have other tests, both on the client and the server ?
Flags: needinfo?(pbrosset)
(In reply to Nicolas Chevobbe from comment #7)
> I implemented what you suggests and I can reproduce the bug anymore.

s/can/can't
(In reply to Nicolas Chevobbe from comment #7)

> So I guess, on the server side, add tests :
> 
> - open a page and the animation inspector, select a node, check the number
> of players, select a node, check the number of player again ( we should not
> have the previous players )
> - open a page and the animation inspector, select a node, check the number
> of players, delete the animation on it, check the number of player again (
> we should not have any players )


I was checking the existing tests server side, and I wonder if https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser_animation_getPlayers.js does not already address this.
(In reply to Nicolas Chevobbe from comment #7)
> So I guess, on the server side, add tests :
> 
> - open a page and the animation inspector, select a node, check the number
> of players, select a node, check the number of player again ( we should not
> have the previous players )
> - open a page and the animation inspector, select a node, check the number
> of players, delete the animation on it, check the number of player again (
> we should not have any players )
> 
> Am I right, or should we have other tests, both on the client and the server
> ?
Not exactly, we need to check that actor references aren't kept alive in the `this.actors` array that the AnimationsActor manages. We need a test that makes sure that `this.actors` contains only the right actors after we've called `getAnimationPlayersForNode`.
This can't be addressed by a test that checks the players returned by `getAnimationPlayersForNode`. This method will always return the right players, but that doesn't tell us if, behind the scenes, we're adding and adding more and more actors and never releasing them.

To do this, you'd need a test that actually can access the internals of the actor, so not a client-side test that must go via the protocol (which, therefore can only call actor methods), but a server test that has access to the actor instance directly and check the contents of `this.actors`.

You can create such a test in \devtools\server\tests\mochitest\
These tests are chrome tests, they run in an HTML page, in a content page (so not in the browser parent process), and have chrome privileges. This means they can instantiate a debugger server connection, get fronts etc. just like the toolbox would do, but since they run in the content process, they can also just get the connection itself and get the actors themselves (not just the fronts).
For instance \devtools\server\tests\mochitest\test_inspector-anonymous.html does something like:

    let serverConnection = gWalker.conn._transport._serverConnection;
    let serverWalker = serverConnection.getActor(gWalker.actorID);

where gWalker is a front, and serverWalker is the corresponding actor.

I suggest creating a test called test_animation_actor-lifetime.html, then copy/paste the boilerplate code from test_inspector-anonymous.html to have something to start with (especially because attachURL is a useful way to start the debugger server and connection without having to do it by hand again).
Then instantiate an AnimationsFront with something like:

let animationsFront = new AnimationsFront(client, tab);

where client and tab are provided by attachURL.
From this, you can call the getAnimationPlayersForNode method as much as you want, and then assert that the corresponding actor's this.actors array is what you expect (by using the conn._transport._serverConnection trick I mentioned before).

I hope this is clear.
I understand there are many new concepts that you don't usually deal with when working on UI bugs only, I'm happy to provide a test if you want me to.
Flags: needinfo?(pbrosset)
Filter on CLIMBING SHOES
Priority: -- → P1
Hi Patrick.

Thank you for taking some time to explain me how things should work. I think the patch fix this issue. I try to add server test with the information you gave me and it seems to work well.
Feel free to tell me if there are things that are wrong in my code.
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

https://reviewboard.mozilla.org/r/35909/#review32645

Looks good to me. If all tests still pass, I think we're good to go!
I've just made a few comments below, only the first one being really important. I can quickly R+ after this.

The only other case which I hadn't thought about previously is if we connect to an older server. Say you connect devtools (with this patch applied) to a remote device that runs an older version of animation.js that doesn't have your changes.
It means that whenever a new node is selected, the old AnimationPlayerActors aren't going to be destroyed. They will still be when the parent AnimationsActor is destroyed, so this shouldn't be a problem.

::: devtools/client/animationinspector/animation-controller.js:187
(Diff revision 1)
>      yield Promise.all(this.nonBlockingPlayerReleases);

You should also remove everything that deals with `nonBlockingPlayerReleases` in this file.
In `onAnimationMutations` the corresponding actors will be removed from `this.animationPlayers` and that's fine, but we don't need to call `release` and store that into an array.

::: devtools/server/tests/mochitest/test_animation_actor-lifetime.html:18
(Diff revision 1)
> +  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

This variable is not used.

::: devtools/server/tests/mochitest/test_animation_actor-lifetime.html:20
(Diff revision 1)
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.webcomponents.enabled", true]
> +  ]});

Why do you need to enable web components for this test?
Attachment #8722211 - Flags: review?(pbrosset)
https://reviewboard.mozilla.org/r/35909/#review32645

I'll try to push a new review at noon.

> Why do you need to enable web components for this test?

Oops, it was in the test I copied it from and I did not removed it. Will do
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35909/diff/1-2/
Attachment #8722211 - Flags: review?(pbrosset)
https://reviewboard.mozilla.org/r/35909/#review32661

::: devtools/client/animationinspector/test/browser_animation_playerFronts_are_refreshed.js
(Diff revision 2)
> -  // Hold on to one of the AnimationPlayerFront objects and mock its release
> -  // method to test that it is released correctly and that its auto-refresh is
> -  // stopped.
> -  let retainedFront = controller.animationPlayers[0];
> -  let oldRelease = retainedFront.release;
> -  let releaseCalled = false;
> -  retainedFront.release = () => {
> -    releaseCalled = true;
> -  };
> -

I thought we could get rid of that as we're testing the player lifecycle server side.
I realize I did not said it : all the client tests in the animationinspector folder passes, so does the new server test.
Attachment #8722211 - Flags: review?(pbrosset) → review+
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

https://reviewboard.mozilla.org/r/35909/#review32871
Attachment #8722211 - Flags: checkin?(pbrosset)
Keywords: checkin-needed
has problems to apply:

patching file devtools/client/animationinspector/animation-controller.js
Hunk #2 FAILED at 167
1 out of 6 hunks FAILED -- saving rejects to file devtools/client/animationinspector/animation-controller.js.rej
patch failed to apply
Flags: needinfo?(chevobbe.nicolas)
(In reply to Carsten Book [:Tomcat] from comment #21)
> has problems to apply:
> 
> patching file devtools/client/animationinspector/animation-controller.js
> Hunk #2 FAILED at 167
> 1 out of 6 hunks FAILED -- saving rejects to file
> devtools/client/animationinspector/animation-controller.js.rej
> patch failed to apply

I'll pull and merge
Flags: needinfo?(chevobbe.nicolas)
Attached patch Bug_1247243.patch (obsolete) — — Splinter Review 
TRY : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e622d41d34d
Attachment #8719169 - Attachment is obsolete: true
Attachment #8722211 - Attachment is obsolete: true
Attachment #8722211 - Flags: checkin?(pbrosset)
Attachment #8723526 - Flags: review+
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35909/diff/2-3/
Attachment #8722211 - Attachment is obsolete: false
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

There were 2 tests that failed because of my patch ( player is null ), because they were using player references that were destroyed due to getAnimationPlayersForNode calls.

All the tests client and server side passes.
Attachment #8722211 - Flags: review+ → review?(pbrosset)
Attachment #8722211 - Flags: review?(pbrosset) → review+
Comment on attachment 8722211 [details]
MozReview Request: Bug 1247243 - Animations are shown only every 2 reloads. r?pbrosset

https://reviewboard.mozilla.org/r/35909/#review33653

Ah I see, by making `getAnimationPlayersForNode` destroy all previously returned `AnimationPlayerActor`s, we've made it impossible for a client to call it several times and keep a reference to all returned values.
This isn't a problem because our only client is the animation-controller.js and it only ever cares about the last time `getAnimationPlayersForNode` is called.
But this could potentially block some future hypothetical use cases where we'd want to show the animations on 2 non-nested nodes.
Let's keep it this way, we've fixed the main issue with this patch, and again, there's no problem with this right now. However, let's add a comment in the jsdoc block of the `getAnimationPlayersForNode` method in animation.js. Something along these lines:

```
  /**
   * Retrieve the list of AnimationPlayerActor actors for currently running
   * animations on a node and its descendants.
   * Note that calling this method a second time will destroy all previously
   * retrieved AnimationPlayerActors. Indeed, the lifecycle of these actors
   * is managed here on the server and tied to getAnimationPlayersForNode
   * being called.
   * @param {NodeActor} nodeActor The NodeActor as defined in
   * /devtools/server/actors/inspector
   */
```

        Attachment #8722211 -
        Attachment is obsolete: true

        Attachment #8723526 -
        Attachment is obsolete: true

        Attachment #8724674 -
        Flags: review+

    
    

    
  
The TRY jobs are over. Errors don't seem related to this patch.
Keywords: checkin-needed

    
    

    
  
For future reference, please try to use commit messages that explain what the patch is actually doing instead of just restating the problem being fixed.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/396a68628a6b
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox47:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: