Feature/bug fix webview state too large #310
Conversation
|
|
||
| class WebViewSessionInMemoryStorage : WebViewSessionStorage { | ||
|
|
||
| private val map = object: LruCache<String, Bundle>(CACHE_SIZE) { |
There was a problem hiding this comment.
Can we rename map to cache?
|
|
||
| /** | ||
| * We can calculate this however we choose, but it should match up with the value we use for cache size. | ||
| * i.e., if we specify max cache size in bytes, we should calculate an approximate size of the cache entry in bytes. |
There was a problem hiding this comment.
I didn't really know what you meant by this comment so I looked up the documentation. Is it worth updating? I have something like this in mind:
Size of a single entry. Refer to the documentation for details on the relationship between size and the max size set in the constructor
| } | ||
|
|
||
| companion object { | ||
| private const val CACHE_SIZE = 10 * 1024 * 1024 // 10 MB |
There was a problem hiding this comment.
Can we change the comment to \\ 10 MiB?
subsymbolic
left a comment
subsymbolic
left a comment
There was a problem hiding this comment.
Looks great. Do you want to hold off merging until @brindy has given it a test too?
| * We can calculate this however we choose, but it should match up with the value we use for cache size. | ||
| * i.e., we specify the max cache size in bytes, so we need to calculate an approximate size of the cache entry in bytes. | ||
| * Size (in bytes) of a single entry in the cache for the given key. | ||
| * We specify the max cache size in bytes, so we need to calculate an approximate size of the cache entry in bytes. |
Yup, sounds good. |
brindy
left a comment
brindy
left a comment
There was a problem hiding this comment.
LGTM!
I also tested with "don't keep activities" turned on
* Store last seen global state; likely a small bug that we weren't already * Wire tab activity and FireDialog to delete web view sessions * Formatting * Update cache size comment to indicate 10 MiB instead of 10 MB * Change variable name from map to cache * Improve comment on sizeOf function
3 participants
Task/Issue URL: https://app.asana.com/0/488551667048375/533891143524678
Tech Design URL:
CC:
Description:
Attempting to save the
WebView's state using the usual Android state saving mechanism can fail with aTransactionTooLargeException. This will happen when the underlyingBundleexceeds a certain size. The size isn't well documented but can be anecdotally anywhere between 500KB and 1MB. This type of exception is dominating our crash reports.This PR stops using the normal Android state saving mechanism, and reverts to an in-memory LRU cache instead. We have chosen 10MiB max cache size (after which
WebViewhistory for least recently used tabs will start becoming inaccessible).Steps to test this PR:
Testing - happy path
HOMEbutton once loadedBACKbutton. Verify the previous page is loaded (and that you don't just land on the new tab screen)Testing - less happy path
WebViewSessionInMemoryStorage.CACHE_SIZEfor this value. Recommend setting it to around 5KB for testing (empirically, each visited site might add around 1KB to the size)Activitiesas soon you leave them from developer settings to force the state saving to kick in.logcatfor output given byWebViewSessionInMemoryStorageclass - this will tell you what the size of the cache is and let you see how close you are to exhausting it.Internal references:
Software Engineering Expectations
Technical Design Template