Closed Bug 1367407 Opened 3 years ago Closed 3 years ago

Intermittent browser_animation_mutations_with_same_names.js | A promise chain failed to handle a rejection: - protocol.js:1212 - Error: Connection closed, pending request to animationplayer48, type getProperties failed

Categories

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

defect

Tracking

(firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: pbro)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

doing a bunch of retriggers:
https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=linux%20pgo%20e10s%20dt&tochange=e52428cb04a0bed3a3a67fe98ec561c608d459aa&fromchange=c0b9404877089ffd6ba3027f761699fa69d6b02c&selectedJob=102341418

top of the list is dt9, then in the middle/bottom it is dt7.

:gl, this had 42 failures yesterday, that is pretty extreme- hopefully we can find a root cause for this- can you get someone from the animation inspector team to look into this?
Flags: needinfo?(gl)
Whiteboard: [stockwell needswork]
this is a side effect of bug 1309468.

:daisuke, I see you authored the patches in bug 1309468, can you please look at this intermittent failure?  I will probably disable this on Tuesday if the failure rate is still high and there is no pending fix.
Blocks: 1309468
Flags: needinfo?(gl) → needinfo?(dakatsuka)
Thanks Joel,
I'll take a look.
this seemed to have stopped around May 29th/30th, do we know why this was "fixed" ?
I haven't modify at all.
I sent to try-server with my debug print since I want to find the cause,  however we could not reproduce the error at this time.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc8e6fb748d6e1c9bad261d92615807b335c4de6
Flags: needinfo?(dakatsuka)
ok, lets see if this picks up again in the future
Whiteboard: [stockwell needswork] → [stockwell unknown]
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Summary: Intermittent browser_animation_mutations_with_same_names.js | A promise chain failed to handle a rejection: - resource://devtools/shared/protocol.js:1212 - Error: Connection closed, pending request to server1.conn8.child1/animationplayer48, type getPrope → Intermittent browser_animation_mutations_with_same_names.js | A promise chain failed to handle a rejection: - protocol.js:1212 - Error: Connection closed, pending request to animationplayer48, type getProperties failed
This started up again on June 3, perhaps with bug 1368364, a backout. Did something go wrong there?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jmaher)
Whiteboard: [stockwell unknown] → [stockwell needswork]
The devtools tests have race conditions on shutdown, and bug 1242505 provided a new mechanism to whitelist them.

After the backout, this can still be whitelisted using the previously existing thisTestLeaksUncaughtRejectionsAndShouldBeFixed function.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jmaher)
Hi Daisuke. Do you have some time to possibly look into this intermittent? It picked up again. Let me know if not and I'll make sure to have someone look into this.
Flags: needinfo?(dakatsuka)
Priority: P3 → P2
Hi Patrick,
I am investigating this one.

About the phenomenon in case of the bug occur:

1. Open inspector
2. Updated animation ui.
3. Call getTracks() in animation-timeline ( also getProperties() of animation in the function )
4. Called getProperties() of animation actor in server.
5. Re-updated animation ui while processing 4? 
   ( in this test case, we use setTimeout() to append animation later. )

Then, the getProperties() of actor in server does not return any result.
So, the connection might be disconnected when animation ui updates?
This is my test try with debug print.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53a000c1b36a36581020cc343b2ddca9d7aa1d5c&selectedJob=105727598
Flags: needinfo?(dakatsuka) → needinfo?(pbrosset)
The problem seems to me that we now have several independent async operations in the panel. When running a test, we don't know which is going to complete first, and our tests don't wait for all of them to be done.
If a test ends before an async operation that led to calling the server could complete, then that leads to the error we're seeing here.
In this particular case, the call to `getTracks` hasn't completed by the time the test ends.

The logs you added to your try push helped a lot! Thanks Daisuke.
I'm going to try a few things and report here if I have a solution for this test.
Flags: needinfo?(pbrosset)
Wao Wao!! No fails!! Thank you so much!!
I'm fixing only this test with this fix here because it's failing so many times it's quite urgent, but I'm fairly sure other animationinspector tests are failing for the exact same reason, so we should move this piece of code to head.js after this is fixed.
Comment on attachment 8876732 [details]
Bug 1367407 - Early return from timeline render if destroy was called;

https://reviewboard.mozilla.org/r/148070/#review152398

Thank you very much, Patrick!
Attachment #8876732 - Flags: review?(dakatsuka) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f8d83753276
Wait for all tracks to be ready in test; r=daisuke
https://hg.mozilla.org/mozilla-central/rev/4f8d83753276
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee: nobody → pbrosset
Alas, not actually fixed, it's still failing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Phil Ringnalda (:philor) from comment #36)
> Alas, not actually fixed, it's still failing.
Are you sure? I've just checked some of the failure logs from comment 35, and they all seem to show logs from before my patch.
Thanks for the URL, I see failures with my patch applied now. This is depressing ... I had triggered many test run on try before landing. I'll take another look.
I've pushed to try with more logs and understood something I had missed the first time around: when there are animation mutations, we re-render the full timeline, but we do this no matter if it's already being rendered or not. We don't wait for async calls to be done then.
Comment on attachment 8876732 [details]
Bug 1367407 - Early return from timeline render if destroy was called;

Mozreview thinks this is a new version of my earlier fix attempt, but it's really a second, different fix. So flagging you for review again on this Daisuke.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c350e512d58d4dc8f8c7c7aafe9ca1085f2fb0f5&group_state=expanded
Attachment #8876732 - Flags: review+ → review?(dakatsuka)
Comment on attachment 8876732 [details]
Bug 1367407 - Early return from timeline render if destroy was called;

https://reviewboard.mozilla.org/r/148070/#review153356

Oh, I see. So the reason is the connection could not handle if the component is destroyed.

And, this patch might resolve bug 1357526 as well!
Thank you very much, Patrich!
Attachment #8876732 - Flags: review?(dakatsuka) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b79157e0ca55
Early return from timeline render if destroy was called; r=daisuke
Please request Beta approval on this patch at your earliest convenience.
Flags: needinfo?(pbrosset)
Comment on attachment 8876732 [details]
Bug 1367407 - Early return from timeline render if destroy was called;

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, this is an intermittent test due to async code racing, that got recently very frequent.

[User impact if declined]: No user impact, but a lot of intermittent test failures in CI.

[Is this code covered by automated tests?]: It is a test.

[Has the fix been verified in Nightly?]: Pushed to autoland so far only. I pushed to try before that, and restarted the test run many many times, without noticing any failures.

[Needs manual test from QE? If yes, steps to reproduce]:  No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No. It's basically an early return in the initialization of a sub-panel in devtools that happens if devtools has already been closed. So it's only cleaning up after ourselves.

[Why is the change risky/not risky?]: See above.

[String changes made/needed]: None.
Flags: needinfo?(pbrosset)
Attachment #8876732 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/b79157e0ca55
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Comment on attachment 8876732 [details]
Bug 1367407 - Early return from timeline render if destroy was called;

devtools fix for intermittent failure, beta55+
Attachment #8876732 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2a945bd4b7ae
Whiteboard: [stockwell needswork] → [stockwell fixed]
Flags: qe-verify-
Whiteboard: [stockwell fixed] → [stockwell fixed:other]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.