1724267 - Re-enable bfcache in parent for AWSY base test (and stop loading about:blank)

Status: NEW

(Testing :: AWSY, task, P3)

Description

[Andrew McCreight [:mccr8]]

I have this disabled now because when Fission was enabled multiple about:blank pages were getting put in a single process.

Comment 1

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

Do you want that about:blanks don't end up to bfcache?
We could tweak https://searchfox.org/mozilla-central/rev/67637695876e27ddbfb6ff99202bb1c951de847a/docshell/base/CanonicalBrowsingContext.cpp#2462
https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/browser_bfcache_exemption_about_pages.js may need some tweaking

Comment 2

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

Does this require some changes to bfcache handling? Or did the recent about:blank changes fix the issue?

Comment 3

[Andrew McCreight [:mccr8]]

What this test does is open a bunch of tabs with about:blank in them. The idea is that if we open N such tabs, then we should have N content processes. I don't know what is needed for that to happen. Nika, was there some specific existing work you are aware of that would fix this issue, or is more needed? Thanks.

Comment 4

[Chris Peterson [:cpeterson]]

This test shouldn't depend on the current bfcache behavior. We should fix the test and then stop disabling bfcache.

Peter will file a new bug to decide whether bfcache should cache about:blank pages or not.

Comment 5

[Andrew McCreight [:mccr8]]

What should the test do instead? I could switch it over to opening actual simple web pages at a bunch of different domains. Or is there something simpler than that that could work?

Comment 6

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

Right now, we create a new tab by calling gBrowser.loadOneTab("about:blank", ...) here: https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/testing/awsy/awsy/awsy_test_case.py#307-312, which does the first process selection, and starts the load of the first about:blank document which ends up in the bfcache, then we end up doing a second navigation to about:blank here: https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/testing/awsy/awsy/awsy_test_case.py#346.

Given the goal of the base memory case, it seems like doing both of these navigations is unnecessary, so I see 2 potential workarounds to avoid it:

1. Pass skipLoad: true to gBrowser.loadOneTab call in https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/testing/awsy/awsy/awsy_test_case.py#308-311. This prevents doing a non-initial about:blank load in that document, meaning that when the navigation later in that function is run, the previous document will be an initial document, and will not enter the bfcache.
2. Tweak how the loads in AWSY tests are performed a bit by skipping the second load if about:blank is the URL to load, and we'd not end up doing a potentially bfcache-invoking navigation.

Comment 7

[Andrew McCreight [:mccr8]]

Thanks for the detailed explanation!

Comment 8

[Chris Peterson [:cpeterson]]

Andrew, I'm tentatively assigning this bug to you because it sounds like you intended to fix it. Is that correct?

This bug is currently tagged as a soft blocker for Fission MVP, but is a lower priority than your one-off Fission memory usage testing.

Comment 9

[Andrew McCreight [:mccr8]]

Yes, that's right.

Comment 10

[Chris Peterson [:cpeterson]]

Reassigning to kmag because he offered to take this bug.

Comment 11

[Chris Peterson [:cpeterson]]

This test fix doesn't need to block Fission MVP.

Comment 12

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 13

[Andrew McCreight [:mccr8]]

fission.bfcacheInParent is still set to false in testing/awsy/conf/base-prefs.json

Comment 14

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

oops

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

@mccr8 can you help me to understand the impact of this bug? Is this something you are able to work on, or can you also provide a summary of what needs to change?

Comment 16

[Andrew McCreight [:mccr8]]

The basic idea here is that the way we're loading pages in the AWSY base test is not entirely realistic.

Olli, maybe you can answer these questions better than me.

It does look like Nika had some possible fixes in comment 6, but nobody has had a chance to look into it.

Comment 17

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

Or we could just not put about:blank to bfcache? We should probably do that anyhow with ship, since there can't be opener relationship.

Comment 18

[Andrej (:aglavic)]

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

Or we could just not put about:blank to bfcache? We should probably do that anyhow with ship, since there can't be opener relationship.

Comment 19

[Andrew McCreight [:mccr8]]

I don't know anything about the BF cache, sorry.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Or we could just not put about:blank to bfcache? We should probably do that anyhow with ship, since there can't be opener relationship.

@nika do you have any thoughts on @smaug's suggestion?

Comment 21

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

:smaug is probably a better person to ask bfcache questions to than me. It seems like it'd be fine ottomh to stop bfcaching about:blank documents. Technically it could cause a programmatically modified about:blank document to lose state, but that's going to happen upon eviction anyway, so it's probably less surprising if we do it unconditionally.