1782247 - Intermittent browser/components/sessionstore/test/browser_pinned_tabs.js waiting for vsync to be disabled (robot.ico animation still running)

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Florian Quèze [:florian]]

While working on bug 1742842, I noticed that sometimes browser/components/sessionstore/test/browser_pinned_tabs.js keeps vsync enabled at the end of the test. I managed to reproduce locally and got a profile: https://share.firefox.dev/3vNdLYP
This shows that the test_selected_pinned_tab_dataloss task kept the robot.ico image animation running. I don't know how that's possible as all tabs are closed at the end of the task: https://searchfox.org/mozilla-central/rev/4a15041348e08fb0d6f5726015c32861e663fbfe/browser/components/sessionstore/test/browser_pinned_tabs.js#108,122,160

Profile where the test correctly cleaned-up: https://share.firefox.dev/3oFkNLe

Comment 1

[Florian Quèze [:florian]]

I debugged this further. Here is a profile of a passing run followed by a failing run, with several debug markers in the "Test" category: https://share.firefox.dev/3vuowPd

When tabbrowser.js removes the tab node (which contains the favicon image), there are 2 possible behaviors in nsImageFrame::DestroyFrom. In the passing case, mKind == Kind::ImageElement and imageLoader->FrameDestroyed is called. In the failing case, mKind is Kind::ContentProperty and we never unregister the image from the refresh driver.

The test passes if there's at least one RefreshDriverTick between when the favicon animation starts and when the tab is removed. During that tick, the style flush destroys the image frame with mKind == ContentProperty and replaces it with an image frame with mKind == Kind::ImageElement (https://share.firefox.dev/3Jn0lYO).

Adding await new Promise(requestAnimationFrame); in the test at https://searchfox.org/mozilla-central/rev/12c2e38fdcc0bcbb9e689560c41ab68f9254413a/browser/components/sessionstore/test/browser_pinned_tabs.js#159 makes the test pass consistently.

It is possible to make the test fail pretty consistently by adding ["gfx.display.frame-rate-divisor", 10], at https://searchfox.org/mozilla-central/rev/12c2e38fdcc0bcbb9e689560c41ab68f9254413a/browser/components/sessionstore/test/browser_pinned_tabs.js#41

I find it strange that the frame kind isn't changed at the time the image animation starts, and only at the next style flush. Emilio, do you know if this is the expected behavior, or who would know?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

It's expected that the frame kind changes whenever style or layout is flushed when an image load starts, yeah.

I guess the bug is that this runs:

https://searchfox.org/mozilla-central/rev/12c2e38fdcc0bcbb9e689560c41ab68f9254413a/dom/base/nsImageLoadingContent.cpp#301-313

And returns a non-null pres context because we have a frame, even though that frame is not for our current request. So FrameDestroyed won't be called and sadness ensues.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Florian, does the attached patch help?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1782247 - Don't track images if our frame is not associated to them. r=tnikkel,aosmond

Comment 5

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Florian, does the attached patch help?

Yes, I can no longer reproduce with the patch applied. Thank you very much!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Do you plan to land the test that would fail without the patch?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cebf54456944
Don't track images if our frame is not associated to them. r=aosmond

Comment 8

[Marian-Vasile Laza]

Backed out changeset cebf54456944 (Bug 1782247) for causing bc failures on browser_windowopen.js.
Backout link
Push with failures <--> bc2
Failure Log

Comment 9

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Do you plan to land the test that would fail without the patch?

The test is an existing one, that already fails intermittently (relatively frequently -- see bug 1782559) since I landed the check that vsync is off after the end of tests (bug 1742842).

Do you think we need to add a test passing/failing deterministically and targeting the issue we identified in this bug specifically?

Comment 10

[Florian Quèze [:florian]]

(In reply to Marian-Vasile Laza from comment #8)

Failure Log

It looks like the new tab favicon now appears one frame later than before. Is this an expected and unavoidable consequence of the patch, or is it pointing to a bug in the implementation?

Comment 11

[Norisz Fay [:noriszfay]]

Backout has been merged to central: https://hg.mozilla.org/mozilla-central/rev/4308b9957543

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Florian Quèze [:florian] from comment #10)

It looks like the new tab favicon now appears one frame later than before. Is this an expected and unavoidable consequence of the patch, or is it pointing to a bug in the implementation?

Seems hard to avoid...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Florian Quèze [:florian] from comment #9)

Do you think we need to add a test passing/failing deterministically and targeting the issue we identified in this bug specifically?

Possibly not. This is pretty hard to test deterministically anyways.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1782247 - Allow another rect for browser_windowopen.js. r=florian

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6391c1d0180a
Allow another rect for browser_windowopen.js. r=florian

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d96a7c14d33
Don't track images if our frame is not associated to them. r=aosmond

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6391c1d0180a
https://hg.mozilla.org/mozilla-central/rev/8d96a7c14d33