Open Bug 1548997 Opened 5 years ago Updated 2 years ago

about:home disables the bf-cache

Categories

(Firefox :: New Tab Page, defect, P3)

Product:
Firefox
Component:
New Tab Page
Type:
defect

Tracking

()

People

(Reporter: ehsan.akhgari, Unassigned, NeedInfo)

References

Details

STR:

  1. Open a new tab.
  2. Click on a link from it.
  3. Click back.

The new tab page "reloads" like a web page. This is specially visible on Nightly with a lot of Pocket recommendations.

Based on a cursory look the reason seems to be that there are two unload event listeners registered on the page:

Scott, can you investigate?

Flags: needinfo?(sdowne)

Yup, I can confirm what Ehsan said, hitting back in AS and DS both re load the page as if it was a page refresh. So like Ehsan said, this is only now more noticeable because of the increase in stories being rendered.

I think our more generic performance fixes would be a higher priority over this, but this is definitely something we can look at doing in 69.

Iteration: --- → 69.1 - May 13 - 26
Flags: needinfo?(sdowne)
Iteration: 69.1 - May 13 - 26 → 69.2 - May 27 - Jun 9
Priority: -- → P1
Iteration: 69.2 - May 27 - Jun 9 → 69.3 - Jun 10 - 23
Assignee: nobody → pdahiya

@wolasi whats the desired behavior here, do we want user to see updated stories on hit of back button. Thanks

Flags: needinfo?(wkonu)

If they hit back i think it's fine to show them the cached version they just left. At some point we'll need a UI solution to indicate to user that new content is available. Could be as simple as a timestamp.

Flags: needinfo?(wkonu)

What do you mean by disables bf-cache? You mentioned the unload listeners, so just having those prevents bf-cache?

Flags: needinfo?(ehsan)

(In reply to Ed Lee :Mardak from comment #5)

What do you mean by disables bf-cache?

"bf-cache" stands for back-forward cache. There is some documentation for this feature available in https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/1.5/Using_Firefox_1.5_caching (it is a rather old feature so that page is pretty outdated, sorry about that.)

The idea behind the bf-cache is that when the user navigates from page 1 to page 2 (e.g. from about:home to a link that they click on from that page) Gecko keeps the state of that page (e.g the JS heap, etc) in memory ready so that if the user presses Back to go back to the previous page (e.g. about:home) we can very quickly replace the old page that's still alive in memory with the current page without performing a full navigation. (Similarly in the Forward direction. That's where the name "bf"-cache comes from.)

You mentioned the unload listeners, so just having those prevents bf-cache?

That's right (see that documentation page.) The reason is that this optimization bends the rules of the web platform that forces the browser to dispatch beforeunload and unload events to the page before the user navigates away. If the page has no listener for these events, it is impossible for the page to realize that we aren't dispatching these events before the user navigates away, so we can enable the bf-cache optimization. If the page does set up one of these listeners, we notice that and disable this optimization because otherwise the listener that the page would have expected to have been called would never be called (as the page wouldn't actually be unloaded when navigating away.) The code that performs this check lives here, FWIW (and as you can see there are many other conditions that determine whether the page can be bf-cached, or IOW whether the presentation of the document can be saved, in Gecko terminology): https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/dom/base/Document.cpp#10179

The result of this for users is degraded performance experience when they go back to about:home after clicking on a link to go away from that page. If about:home removed its unload event listeners and got bf-cached successfully, when the user pressed back, the page would show up instantly without any noticeable delay.

Flags: needinfo?(ehsan)

Thanks for the details. At least for the 2 unloads noted earlier:

(In reply to :Ehsan Akhgari from comment #0)

This is only added with browser.tabs.remote.separatePrivilegedContentProcess set to true (only default true on nightly). Although quick tests seem to show this unload never gets called… ??

This one is added for any content process page with special messaging privileges as RemotePageManager needs to clean up its state/references. Additionally, the unload message is forwarded to activity stream where we dispatch NEW_TAB_UNLOAD which we use for various cleanup but probably most importantly to send a session ping.

I'm not sure if it's feasible to get rid of this unload listener, and I would guess any alternative would still need something like the unload listener anyway. Or do you have ideas on how we can avoid the listeners for this?

(In reply to Ed Lee :Mardak from comment #7)

Thanks for the details. At least for the 2 unloads noted earlier:

(In reply to :Ehsan Akhgari from comment #0)

This is only added with browser.tabs.remote.separatePrivilegedContentProcess set to true (only default true on nightly). Although quick tests seem to show this unload never gets called… ??

So I guess this should block bug 1513045.

This one is added for any content process page with special messaging privileges as RemotePageManager needs to clean up its state/references. Additionally, the unload message is forwarded to activity stream where we dispatch NEW_TAB_UNLOAD which we use for various cleanup but probably most importantly to send a session ping.

I'm not sure if it's feasible to get rid of this unload listener, and I would guess any alternative would still need something like the unload listener anyway. Or do you have ideas on how we can avoid the listeners for this?

It really depends on the nature of the thing that this listener is doing, that is, whether that thing is expected to happen when the user clicks on a link on about:home (or another page which gets this listener applied to it) to navigate to another page or not. If the answer to that is yes, one option would be to move this work to the pagehide listener instead of the unload listener. The pagehide listener is fired when a page is navigated away from, and its event object also has a persisted attribute that will be set to true if the page is about to be put into the bf-cache. (There is a symmetric pageshow event for when the page is navigated to or back to again.)

Blocks: 1513045

Discussed briefly with team, user pressed back button is not a common case and this optimization involves

  • refactoring to remove unload listeners and use pagehide.
  • possible memory overhead
  • changes in interaction with Remote Manager

Prior to 57 back button was disabled for newtab and is one option. Considering complete fix is blocked on 1513045, propose revisiting this bug in later release cycles.

Tawanda, I second Punam's recommendation to move this to our backlog as a P3 (it's definitely not going to get fixed this release). Any objections?

Flags: needinfo?(tkanhema)
Assignee: pdahiya → nobody
Iteration: 69.3 - Jun 10 - 23 → ---
Priority: P1 → P3
Component: Activity Streams: Newtab → New Tab Page

Ok to make this a P3

Flags: needinfo?(tkanhema)

(In reply to Punam Dahiya [:pdahiya] from comment #9)

Discussed briefly with team, user pressed back button is not a common case

Are you saying that users pressing the back button after navigating away from about:home is not a common case? Do we have telemetry data that was used to arrive at this conclusion?

and this optimization involves

  • refactoring to remove unload listeners and use pagehide.
  • possible memory overhead

Could you please elaborate on the memory overhead concern?

(In reply to :Ehsan Akhgari from comment #12)

(In reply to Punam Dahiya [:pdahiya] from comment #9)

Discussed briefly with team, user pressed back button is not a common case

Are you saying that users pressing the back button after navigating away from about:home is not a common case? Do we have telemetry data that was used to arrive at this conclusion?

NI Nan to help point to telemetry numbers if available. If numbers indicate high usage, I agree we should revisit this in 70. Thanks!

and this optimization involves

  • refactoring to remove unload listeners and use pagehide.
  • possible memory overhead

Could you please elaborate on the memory overhead concern?

Running about:memory shows ~7 MB increase memory usage with each opened new tab in privileged content process.
Memory usage increase seen with 2 tabs vs 3 tabs open

https://gist.github.com/punamdahiya/88b6f4d816c44e84aa1477c65573c37c
https://gist.github.com/punamdahiya/0110e10c827109fce3bf208f3ca36fe0

My understanding without explicit unload navigating away from new tab will keep this memory occupied. NI Ed in case I am missing some additional info here. Thanks

Flags: needinfo?(najiang)
Flags: needinfo?(edilee)

(In reply to Punam Dahiya [:pdahiya] from comment #13)

Are you saying that users pressing the back button after navigating away from about:home is not a common case? Do we have telemetry data that was used to arrive at this conclusion?

NI Nan to help point to telemetry numbers if available. If numbers indicate high usage, I agree we should revisit this in 70. Thanks!

Unfortunately, this particular load reason ("hitting the back button") was taken out since the Quantum release. We used to collect it, but it was so uncommon for the users to start the about:home/newtab session by clicking the back button compared to other session triggers, iirc that's why we decided not to include it in the telemetry in 57.

Flags: needinfo?(najiang)

(In reply to Nan Jiang [:nanj] from comment #14)

(In reply to Punam Dahiya [:pdahiya] from comment #13)

Are you saying that users pressing the back button after navigating away from about:home is not a common case? Do we have telemetry data that was used to arrive at this conclusion?

NI Nan to help point to telemetry numbers if available. If numbers indicate high usage, I agree we should revisit this in 70. Thanks!

Unfortunately, this particular load reason ("hitting the back button") was taken out since the Quantum release. We used to collect it, but it was so uncommon for the users to start the about:home/newtab session by clicking the back button compared to other session triggers, iirc that's why we decided not to include it in the telemetry in 57.

Hmm, comment 9 mentions that before 57 the back button was disabled for about:home, so perhaps that's why the numbers were so low?

(In reply to :Ehsan Akhgari from comment #15)

Hmm, comment 9 mentions that before 57 the back button was disabled for about:home, so perhaps that's why the numbers were so low?

That telemetry was only for about:newtab. Sorry for the confusion.

Here is a snapshot on 2019-07-02 about the distribution of the load trigger types for about:home. The first_window_opened is for the about:home startup page, unexpected includes various triggers, such as typing "about:home" in the address bar, clicking the "home" toolbar, opening a new window, and clicking the "back" button.

+---------------------+----------+
| load_trigger_type | sessions |
|---------------------+----------|
| first_window_opened | 72,619,892 |
| unexpected | 24,828,969 |
+---------------------+----------+

Thanks for the extra explanation, Nan! So that's about 34% of loads if I understand correctly, but since there is no breakdown data available hard to say how much of that specifically came from history (back/forward) navigation.

No longer blocks: pocket-newtab
No longer blocks: 1513045
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.