Closed Bug 1782247 Opened 16 days ago Closed 10 days ago

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

Categories

(Firefox :: Session Restore, defect)

defect

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: florian, Assigned: emilio)

References

Details

Attachments

(2 files)

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

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?

Flags: needinfo?(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.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

Florian, does the attached patch help?

Flags: needinfo?(florian)
Severity: -- → S3

(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!

Flags: needinfo?(florian)

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

Flags: needinfo?(florian)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cebf54456944
Don't track images if our frame is not associated to them. r=aosmond

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

Flags: needinfo?(emilio)

(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?

Flags: needinfo?(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?

(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...

(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.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6391c1d0180a
Allow another rect for browser_windowopen.js. r=florian
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d96a7c14d33
Don't track images if our frame is not associated to them. r=aosmond
Regressions: 1783066
Status: NEW → RESOLVED
Closed: 10 days ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
You need to log in before you can comment on or make changes to this bug.