1513938 - TransactionTooLargeException when app with a Private tab is put in background

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

Even after the patch from bug 1480852 we are still seeing TransactionTooLargeException s apparently from when the application is put in background.

Interesting user reports: 
> f5330577-f37c-4fc2-b8d8-18fea0181213
> 1af6bf3a-e3c6-4c25-8c05-9a8b60181210
> 36d560dd-7ac2-44c7-9bb0-8b8ae0181210
> ea90db37-a1f5-4ce0-bd43-600c80181210
> 9081f935-9ca2-4745-9239-bd15e0181210
> 59057c35-d84e-4ec8-9e2d-f4f160181209
> 96056589-61a1-49bb-8fdd-dc5ce0181208

> eb432f81-6bca-46af-b7a5-186be0181208
> 59fec5cd-65e0-44de-b3b0-1a4620181209
> 1cf80383-7c95-4f8a-b607-f9bad0181210

> 46d91880-3dff-4c58-8c4c-538590181210
> 744c4309-9c0a-4a3f-bd17-2e0a60181209

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Petru, sorry but I edited your message #0 to remove the comments from users. They are confidential.

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Found an easy way to replicate this:
- open a "New private tab"
- go to amazon.com
- open a product listing
- press home/recents

This happens because we try to keep the private session in memory, ready for the next app start [1]

> BrowserApp.onSaveInstanceState wrote: Bundle@184415016 contains 7 keys and measures 748.8 KB when serialized as a Parcel
>        * android:support:fragments = 1.4 KB
>        * inBackground = 0.0 KB
>        * android:fragments = 0.4 KB
>        * dynamic_toolbar = 0.1 KB
>        * abouthome_top_padding = 0.1 KB
>        * privateSession = 746.8 KB
>        * android:lastAutofillId = 0.1 KB

I see the private tabs data is treated differently so I'll look into what's the best possible solution for avoiding to clog the Binder with too much data [2]
> The Binder transaction buffer has a limited fixed size, currently 1Mb, which is shared by all transactions in progress for the process. Consequently this exception can be thrown when there are many transactions in progress even when most of the individual transactions are of moderate size. 

Currently my options are
- persisting the data on disk - this will introduce a minor delay in app's usual flow
- dropping the `privateSession` state if it is bigger than an arbitrary value of 300 KB as with `android:viewHierarchyState` [3]. Need to investigate it's impact. 

[1] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#637
[2] https://developer.android.com/reference/android/os/TransactionTooLargeException
[3] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2303

Comment 3

[Petru-Mugurel Lingurar [:petru]]

After investigating this more it seems like we want to be able to persist the private browsing session between OOMs with the help of the onSaveInstanceState Bundle. This being sensitive data, it is not persisted to disk like in the case of the normal tabs.

So _currently_ the only remaining solution to not crash would remain to not put the private browsing session in that Bundle, with a small possible optimization being that we would only do this if that session is above a certain size.

Looking to see why that state has such a big size I saw that the `structuredCloneState` field is the culprit.
But I couldn't gather much info about it from https://wiki.mozilla.org/Firefox/session_restore

Since a better solution would involve reducing the size of that field _if possible_ I'm NI-ing Susheel to maybe find someone to know about the `Session Restore` functionality.

Comment 4

[:sdaswani]

James, is the private browsing session saved to memory as part of Gecko, or is there something on the 'front end' that handles that?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :sdaswani only needinfo from comment #4)
> James, is the private browsing session saved to memory as part of Gecko, or
> is there something on the 'front end' that handles that?

It appears to be a mix. The data comes from SessionStore, which is mostly a Gecko thing, but Fennec is responsible for storing it somewhere. I think my suggestion to avoid this problem may be to store it outside of the Bundle, and only store some key for accessing the data in the bundle.

Comment 6

[Jan Henning [:JanH]]

If the comments in SessionHistory.jsm [1]/nsISHEntry.idl [2] are still accurate, the structuredCloneState seems to be the serialisation of data that sites have stored using the pushState API (https://developer.mozilla.org/en-US/docs/Web/API/History_API#Adding_and_modifying_history_entries).

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> I think my suggestion to avoid this problem may be to store it outside of the Bundle,
> and only store some key for accessing the data in the bundle.

To clarify, you mean encrypting the private browsing data with a random key, writing the data itself to storage, but storing the encryption key as part of the savedInstance state?

Yes, that would sound like an acceptable idea. The only difference compared to today that I can think of is that in case someone gains access to our data directory, the existence and size of a private browsing session store file would be leaked. The contents however would be as safe as they are today - if somebody could retrieve the key from the saved instance state as well, today the could also simply directly get the private browsing data itself.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/modules/sessionstore/SessionHistory.jsm#226
[2] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/docshell/shistory/nsISHEntry.idl#160

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jan Henning [:JanH] from comment #6)
> If the comments in SessionHistory.jsm [1]/nsISHEntry.idl [2] are still
> accurate, the structuredCloneState seems to be the serialisation of data
> that sites have stored using the pushState API
> (https://developer.mozilla.org/en-US/docs/Web/API/
> History_API#Adding_and_modifying_history_entries).
> 
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> > I think my suggestion to avoid this problem may be to store it outside of the Bundle,
> > and only store some key for accessing the data in the bundle.
> 
> To clarify, you mean encrypting the private browsing data with a random key,
> writing the data itself to storage, but storing the encryption key as part
> of the savedInstance state?
> 

No I mean continue keeping it in memory somewhere, just not in the Bundle. A HashMap somewhere, with the key stored in the Bundle.

Comment 8

[Jan Henning [:JanH]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> No I mean continue keeping it in memory somewhere, just not in the Bundle. A
> HashMap somewhere, with the key stored in the Bundle.

But the reason we're storing the data in the savedInstanceState in the first place is because we wanted something that *survives an OOM kill*, yet still is as ephemeral as possible. The savedInstanceState fits the bill quite well - it is explicitly intended for state that needs to survive OOM kills, it is kept securely by the OS [1], but doesn't persist if the user explicitly kills us (through the activity chooser or by force-stopping) or beyond a reboot.

If we store the data "somewhere" else *in memory*, it wouldn't survive an OOM kill, would it? Or is there some trick that I don't know about?

[1] Although I've got no idea whether the data is really kept in memory only, or possibly and temporarily written to the storage, too...

Comment 9

[Petru-Mugurel Lingurar [:petru]]

So my idea was that someone could investigate if it's possible to reduce the size of that `structuredCloneState` on the Gecko side, before we store it somewhere, because it only happens with specific websites.

For example it's very easy to reproduce this by going from the Amazon's homepage to a product listing
(in which case the `structuredCloneState` for the `https://www.amazon.com/` entry would have hundreds of KBs which will crash the app when we try to send it in the Bundle -  https://drive.google.com/file/d/1fzyWTwZNR65T_wMPUckmcBzCMeF-DykF/view?usp=sharing) 
but with other websites the entire session would only have a few tens of KBs.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1513938 - Enforce a Bundle size limit and drop `privateSession` if exceeds it; r?JanH

The `privateSession` key would normally allow persisting the Private Browsing
session across OOMs in Activity's Bundle.
We need to do that to avoid storing private, sensible data on disk like we do
with the normal browsing session.

In some cases `privateSession` would contain a lot of data which, along with
other possible concurrent transactions could overflow Binder's buffer which has
a limited fixed size, currently 1Mb.
To avoid this, we will drop `privateSession` from the Bundle if the resulting
size is greater than a _speculative_ size of 300KBs which would mean that in
the case of an OOM all Private Browsing state would be lost.

Bug 1515592 is filed to investigate for a better solution.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f4436e2493a
Enforce a Bundle size limit and drop `privateSession` if exceeds it; r=JanH

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4f4436e2493a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Looking good on Nightly so far. Jan, can you please request Beta approval on this since Petru is on PTO? Thanks!

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 9032631 [details]
Bug 1513938 - Enforce a Bundle size limit and drop `privateSession` if exceeds it; r?JanH

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Private browsing/session store

User impact if declined: If the user has a private browsing session with a large amount of session history data, Firefox may crash when it goes into the background.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: see comment 2

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch is just dropping the private browsing session store data if it exceed a certain amount. This doesn't lead to an ideal user experience (because the user's private browsing tabs will be lost if we're OOM-killed later on), but it still is better than crashing.

String changes made/needed: none

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9032631 [details]
Bug 1513938 - Enforce a Bundle size limit and drop `privateSession` if exceeds it; r?JanH

Fix for #1 top crash for Fennec, let's uplift.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

We may also want to take this on release in case we end up with a 64 dot release. It could be a release driver in itself since it's the #1 top crash by far on fennec.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/226db4f6cb06

Comment 18

[Levente Sacal]

Marking 65 as fixed as I couldn't reproduce the issue by following the steps from comment #9.

Comment 19

[Julien Cristau [:jcristau]]

Jan or Petru, what do you think about getting this in to 64.0.2? Please request uplift to release if you think it appropriate (crash volume on beta seems to have dropped massively in 65.0b8).

Comment 20

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9032631 [details]
Bug 1513938 - Enforce a Bundle size limit and drop privateSession if exceeds it; r?JanH

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Private browsing/session store

User impact if declined: If the user has a private browsing session with a large amount of session history data, Firefox may crash when it goes into the background.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: see comment #2 / comment #9

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch is just dropping the private browsing session store data if it exceed a certain amount. This doesn't lead to an ideal user experience (because the user's private browsing tabs will be lost if we're OOM-killed later on), but it still is better than crashing.

String changes made/needed:

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 9032631 [details]
Bug 1513938 - Enforce a Bundle size limit and drop privateSession if exceeds it; r?JanH

address fennec top crasher, approved for 64.0.2. Thanks!

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/65621d0fe126

Comment 23

[Sorina Florean [:sflorean]]

Couldn't reproduce the issue by following the steps from comment #9 on 64.0.2 RC Build 1. Marking 64 as verified.