1798780 - Blank tab and tab switching hang within a window on back/forward navigation in cache

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Chris H-C :chutten]

STR:
On latest nightly 20221102094537 on any platform (Windows, Mac, Linux all tested)

1. Load https://developer.mozilla.org/ (It happened first for me on the Rust docs, but I have cleaner repro on MDN)
2. Select Reference > CSS. Expand Properties. Select any property to visit its page.
3. Select a few items from the Table of Contents on the Right.
4. Long press Back and select CSS
5. Long press Forward and select the most recent item

The tab goes white and the whole window (but only that window. Other windows are fine) refuses to allow tab switching. UI items are still interactable, though the back and forward buttons when long-pressed do not show any content (just a small (2x11px ish?) blank popup).

To get out of it I open a new window (which works fine) and then use about:restartrequired to restart the session. The session restarts with the tab no longer white and displaying the content I'd selected in Step 5.

Lemme get a perf profile.

Comment 1

[Chris H-C :chutten]

Here's a profile that starts at Step 4: https://share.firefox.dev/3Dw8JTw

Comment 2

[Chris H-C :chutten]

Mozregression gives me https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=383986e2f5cb835a55c47bbd6e15d81035ca0784&tochange=d6e8528f0a936df88369c71f4e390e31a4d621a1 which rather unfortunately points the finger at Fission.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

This looks like some sort of BFCache failure. I was able to reproduce the specific issue locally.

ni? :smaug to potentially look into what's falling over here?

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Setting Regressed by field after analyzing regression range found by mozregression in comment #2.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1732358

Comment 6

[Pascal Chevrel:pascalc]

We are in 107 RC week, wontfix 106. Could we get a severity set to this bug to help with regression triage? Thanks

Comment 7

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

I can't reproduce.
Very exact steps might be useful here.

Does it matter if fission.bfcacheInParent is enabled or disabled?

Comment 8

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

peterv, can you try to reproduce?

Comment 9

[Chris H-C :chutten]

If it helps, fission.bfcacheInParent is true on the profiles I used to reproduce the issue.

More precise STR:

1. Load developer.mozilla.org
2. Hover Reference > click CSS
3. Expand Properties, click -webkit-line-clamp
4. Click "Formal definition" then click "Examples" then click "See also" from the TOC on the right.
5. Using the mouse, long-press the Back button (History should be (from bottom to top) "New Tab", "MDN Web Docs", "CSS", and then four -webkit-line-clamp entries)
6. Select the history item for the CSS page. (Third from the bottom)
7. Using the mouse, long-press the Forward button and select the topmost history item (the one for "See also")

Comment 10

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

Does the issue disappear if fission.bfcacheInParent is false. (It is true by default)

Comment 11

[Chris H-C :chutten]

Seems to work as intended with fission.bfcacheInParent is set to false, yes. No more blank content, no more hang.

Comment 12

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

Aha, now it happened, with that exact STR. Thanks.

Peterv, would you have time to take a look at this?

Comment 13

[Donal Meehan [:dmeehan]]

:smaug could we get a severity on this?

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1732358

Comment 15

[Jens Stutte [:jstutte]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #12)

Aha, now it happened, with that exact STR. Thanks.

Peterv, would you have time to take a look at this?

Renewing the ni? to Peter.

Comment 16

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

I said (during SHIP meeting) that I'll take a look

Comment 17

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

Attachment: Bug 1798780, ensure we don't evict the current frameloader, r=peterv

Comment 18

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

I think we do want this on esr and beta too

Comment 19

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

The patch applies cleanly to beta, esr102 needs just a tweak to browser.ini

Comment 20

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

Attachment: Bug 1798780, ensure we don't evict the current frameloader, for esr102, r=peterv

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22f9c1876fe5
ensure we don't evict the current frameloader, r=peterv

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/22f9c1876fe5

Comment 23

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

Comment on attachment 9305922 [details]
Bug 1798780, ensure we don't evict the current frameloader, r=peterv

Beta/Release Uplift Approval Request

• User impact if declined: Broken browser window if back/forward is used in a particular way on pages which use bfcache.
  The issue have been there on the Nightly for 18 months I think, so, not a new regression, but a bad one.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: NA
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I think this should be relatively low risk, since it just prevents evicting a page from bfcache at certain moment, but it will be evicted later.

(Android isn't affected, because it uses still the old session history implementation)

• String changes made/needed: NA
• Is Android affected?: No

Comment 24

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

Comment on attachment 9306064 [details]
Bug 1798780, ensure we don't evict the current frameloader, for esr102, r=peterv

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Broken browser window if back/forward is used in a particular way on pages which use bfcache.
  The issue have been there on the Nightly for 18 months I think, so, not a new regression, but a bad one.
• User impact if declined: See above
• Fix Landed on Version: 109
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I think this should be relatively low risk, since it just prevents evicting a page from bfcache at certain moment, but it will be evicted later.

Comment 25

[Dianna Smith [:diannaS]]

Comment on attachment 9305922 [details]
Bug 1798780, ensure we don't evict the current frameloader, r=peterv

Approved for 108.0b9

Comment 26

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e37965741f75

Comment 27

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest Nightly build as well as beta 108.0b9.

Comment 29

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

[Tracking Requested - why for this release]:
If we see more reports on 107 (because some web sites have changed now? This isn't really a new regression in the behavior) and something prevents us to ship 108 on time, we should consider to land this to 107 too. Or if we do a dot release for some other reason.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

We're building RCs on Monday. There won't be another 107 build unless it's a chemspill and this would be out of scope for that anyway :)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9306064 [details]
Bug 1798780, ensure we don't evict the current frameloader, for esr102, r=peterv

Approved for 102.6esr.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/5cf04e23dbbf

Comment 33

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest build 102.6.0esr.