Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Avoid eagerly rehydrating EngineSessionState for recently closed tabs #11688

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Feb 9, 2022

This change splits out tab-specific data from RecoverableTab data class
into a separate TabState (so, it doesn't have the EngineSessionState).

Then, once we have the simplified TabState, everything that touches RecentlyClosedTabs
is converted to use that instead of its more expensive sibling.

This way we avoid having to eagerly process EngineSessionState simply to populate BrowserState.closedTabs.
This saves us from having to hit disk (where the EngineSessionState is persisted) on initializing BrowserState (so, startup in most cases).
It also saves us from having to parse/rehydrate that persisted state, and keep it in memory (in some regressive cases, these EngineSessionStates are very large).

At the point we actually need the EngineSessionState for a tab we'd like to restore, we can read/process it.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@grigoryk
Copy link
Contributor Author

grigoryk commented Feb 9, 2022

Fenix PR: mozilla-mobile/fenix#23649
Looks like R-B doesn't use this stuff, so no changes necessary there.

@grigoryk
Copy link
Contributor Author

grigoryk commented Feb 9, 2022

Most of this PR is really just churn around pushing tab state down into RecoverableTab.state, but alas.

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2022

This pull request has conflicts when rebasing. Could you fix it @grigoryk? 🙏

@csadilek csadilek self-assigned this Feb 9, 2022
@grigoryk grigoryk force-pushed the lazyStateInTabs branch 3 times, most recently from 273c45d to 8f57c94 Compare February 10, 2022 11:27
@grigoryk
Copy link
Contributor Author

Snuck in a few other things into this PR:

  • cleaning up the tests to remove all the weird experimental coroutine stuff and expanded them to cover interaction with the engine session state storage
  • relaxed behaviour of the recentlyclosedtabsstorage around engine state persistence failures
  • addressed @csadilek 's feedback around making an effort to hide EngineSessionStateStorage impls from the application

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this in both A-C sample browser and Fenix. I think this is ready to land.

One thing we should discuss is the issue Mihai linked (independent), but we could handle here as well. A crash reading from the recently closed tabs storage should never bring down the app. Let's chat but we can land this.

@grigoryk grigoryk force-pushed the lazyStateInTabs branch 3 times, most recently from 8e3ac29 to db9c9d2 Compare February 11, 2022 01:00
@grigoryk
Copy link
Contributor Author

Added try/catch around getTabs and addTabsToCollectionWithMax - this will get us to stop crashing in cases such as mozilla-mobile/fenix#23349.
Additionally, if something goes wrong with the file storage layer of this, we'll no longer crash on startup as well (since that's now off the startup path).

Added a bit more RecentlyClosedTabsStorage tests and tests for the refactored FileEngineSessionStateStorage.

@grigoryk grigoryk added the 🛬 needs landing PRs that are ready to land label Feb 14, 2022
@grigoryk grigoryk closed this Feb 14, 2022
@grigoryk grigoryk reopened this Feb 14, 2022
@grigoryk
Copy link
Contributor Author

Hmmm, this didn't fail before:

[task 2022-02-14T18:53:16.751Z]   TEST: testRemovingAllTabs
[task 2022-02-14T18:53:16.751Z]     [Robolectric] mozilla.components.feature.recentlyclosed.RecentlyClosedTabsStorageTest.testRemovingAllTabs: sdk=28; resources=binary
[task 2022-02-14T18:53:16.751Z]     I/MonitoringInstr: Instrumentation started!
[task 2022-02-14T18:53:16.751Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.751Z]     D/SQLiteCursor: received count(*) from native_fill_window: 1
[task 2022-02-14T18:53:16.752Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.752Z]     D/SQLiteCursor: received count(*) from native_fill_window: 1
[task 2022-02-14T18:53:16.752Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.752Z]     D/SQLiteCursor: received count(*) from native_fill_window: 1
[task 2022-02-14T18:53:16.752Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.752Z]     D/SQLiteCursor: received count(*) from native_fill_window: 0
[task 2022-02-14T18:53:16.850Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.850Z]     D/SQLiteCursor: received count(*) from native_fill_window: 0
[task 2022-02-14T18:53:16.850Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.850Z]     D/SQLiteCursor: received count(*) from native_fill_window: 2
[task 2022-02-14T18:53:16.850Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.850Z]     D/SQLiteCursor: received count(*) from native_fill_window: 0
[task 2022-02-14T18:53:16.850Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.850Z]     D/SQLiteCursor: received count(*) from native_fill_window: 2
[task 2022-02-14T18:53:16.850Z]     I/CursorWindowStats: Created a new Cursor. # Open Cursors=1 (# cursors opened by this proc=1)
[task 2022-02-14T18:53:16.850Z]     D/SQLiteCursor: received count(*) from native_fill_window: 0
[task 2022-02-14T18:53:16.850Z]   FAILURE
[task 2022-02-14T18:53:16.850Z] 
[task 2022-02-14T18:53:16.854Z] org.junit.ComparisonFailure: expected:<https://[mozilla.org]> but was:<https://[pocket.com]>
[task 2022-02-14T18:53:16.854Z] 	at org.junit.Assert.assertEquals(Assert.java:115)
[task 2022-02-14T18:53:16.854Z] 	at org.junit.Assert.assertEquals(Assert.java:144)

@grigoryk
Copy link
Contributor Author

Ah, it's failing intermittently locally as well - we're getting a set of stored tabs back, ordered by lastAccess, and in a test the order isn't stable (since System.currentTimeMillis() will likely give us the two identical values for both tabs under test).

This change splits out tab-specific data from RecoverableTab data class
into a separate TabState (so, it doesn't have the EngineSessionState).

Then, once we have the simplified TabState, everything that touches RecentlyClosedTabs
is converted to use that instead of its more expensive sibling.

This way we avoid having to eagerly process EngineSessionState simply to populate BrowserState.closedTabs.
This saves us from having to hit disk (where the EngineSessionState is persisted) on initializing BrowserState (so, startup in most cases).
It also saves us from having to parse/rehydrate that persisted state.

At the point we actually need the EngineSessionState for a tab we'd like to restore, we can read/process it.
@grigoryk grigoryk merged commit ac2215c into mozilla-mobile:main Feb 14, 2022
@grigoryk grigoryk deleted the lazyStateInTabs branch February 14, 2022 22:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants