Closed Bug 1513938 Opened 6 years ago Closed 6 years ago

TransactionTooLargeException when app with a Private tab is put in background

Categories

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

Product:
Firefox for Android Graveyard
Component:
General
Platform:
Other
Android
Type:
defect

Tracking

(firefox64+ verified, firefox65+ verified, firefox66 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox64 + verified
firefox65 + verified
firefox66 --- verified

People

(Reporter: petru, Assigned: petru)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

Bug 1513938 - Enforce a Bundle size limit and drop `privateSession` if exceeds it; r?JanH
(deleted), text/x-phabricator-request
lizzard
: approval-mozilla-beta+
jcristau
: approval-mozilla-release+
Details
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
Blocks: 1503255
Crash Signature: [@ android.os.TransactionTooLargeException: at android.os.BinderProxy.transactNative(Native Method)]
status-firefox64: --- → affected
status-firefox65: --- → affected
status-firefox66: --- → affected
Keywords: crash
Product: Firefox → Firefox for Android
Comment 0 is private: false
Petru, sorry but I edited your message #0 to remove the comments from users. They are confidential.
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
Summary: TransactionTooLargeException when Activity is stopped → TransactionTooLargeException when app with a Private tab is put in background
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.
Flags: needinfo?(sdaswani)
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
James, is the private browsing session saved to memory as part of Gecko, or is there something on the 'front end' that handles that?
Flags: needinfo?(sdaswani) → needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
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) (jwillcox@mozilla.com) 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
(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) (jwillcox@mozilla.com) 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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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...
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.
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.
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4f4436e2493a Enforce a Bundle size limit and drop `privateSession` if exceeds it; r=JanH
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f4436e2493a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Looking good on Nightly so far. Jan, can you please request Beta approval on this since Petru is on PTO? Thanks!
Flags: needinfo?(jh+bugzilla)
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
Flags: needinfo?(jh+bugzilla)
Attachment #9032631 - Flags: approval-mozilla-beta?
Depends on: 1515592
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.
Attachment #9032631 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
tracking-firefox64: --- → ?
tracking-firefox65: --- → +
Marking 65 as fixed as I couldn't reproduce the issue by following the steps from comment #9.
status-firefox65: fixed → verified
Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified

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).

tracking-firefox64: ? → +
Flags: needinfo?(petru.lingurar)

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:

Flags: needinfo?(petru.lingurar)
Attachment #9032631 - Flags: approval-mozilla-release?

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!

Attachment #9032631 - Flags: approval-mozilla-release? → approval-mozilla-release+

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

status-firefox64: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: