Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663413002:
[WebView shell] Discard very long WebView states (Closed)
|
Created:
3 years, 10 months ago by Tima Vaisburd Modified:
3 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WebView shell] Discard very long WebView states
WebView serializes its state into Android's Bundle so it can restore
it through activity recycle. Since N Android has inforced the limit
for the size of the bundle, at the same time the length of the serialized
state has grew beyond this limit.
This CL puts a temorary fix to this problem into SystemWebViewShell only.
The states longer than 300k are now dropped by the shell.
BUG=686607
Review-Url: https://codereview.chromium.org/2663413002
Cr-Commit-Position: refs/heads/master@{#447681}
Committed: https://chromium.googlesource.com/chromium/src/+/c58f0323797ee837ccc917c8e31886a0fcbf712e
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added Toast #
Total comments: 2
Patch Set 3 : Added the actual legth to the toast message #
Messages
Total messages: 16 (6 generated)
timav@chromium.org changed reviewers: + boliu@chromium.org, timvolodine@chromium.org, torne@chromium.org
This is my attempt at dealing with the TransactionTooLongException. I know Torne was against this kind of approach because it does nothing but masking the problem, but I though that the WevView shell is used extensively for tests and simply eliminating the crash might have a value. I also considered a Fragment with setRetainedInstance(true) but it seems the latter would only be good for config changes and not memory pressure kills. PTAL.
https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:227: byte[] webViewState = savedInstanceState.getByteArray(SAVE_RESTORE_STATE_KEY); this is a bit hacky new a bundle, have webview fill that, then conditionally put it into savedInstanceState with putBundle. then need to update restore to match
https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:227: byte[] webViewState = savedInstanceState.getByteArray(SAVE_RESTORE_STATE_KEY); On 2017/02/01 17:38:26, boliu wrote: > this is a bit hacky > > new a bundle, have webview fill that, then conditionally put it into > savedInstanceState with putBundle. then need to update restore to match Are you trying to avoid exposing SAVE_RESTORE_STATE_KEY? I did not find a Bundle method that returns the total length of all fields, would have to calculate manually, but I think can do. If not for this reason, why?
Doing this *specifically in the shell* is okay; I was objecting to doing it in WebView itself. However, can we pop up a toast or something when we drop the state on teh floor, just so we can see when it happens in testing?
+1 for toast https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2663413002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:227: byte[] webViewState = savedInstanceState.getByteArray(SAVE_RESTORE_STATE_KEY); On 2017/02/01 17:48:44, Tima Vaisburd wrote: > On 2017/02/01 17:38:26, boliu wrote: > > this is a bit hacky > > > > new a bundle, have webview fill that, then conditionally put it into > > savedInstanceState with putBundle. then need to update restore to match > > Are you trying to avoid exposing SAVE_RESTORE_STATE_KEY? I did not find a Bundle > method that returns the total length of all fields, would have to calculate > manually, but I think can do. If not for this reason, why? Ahh, good point. Never mind then
Added Toast saying "Can't save state: too long".
one more suggestion, but lgtm https://codereview.chromium.org/2663413002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2663413002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:231: Toast.makeText(this, "Can't save state: too long", Toast.LENGTH_SHORT).show(); maybe include the actual length? round to kb maybe?
https://codereview.chromium.org/2663413002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2663413002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:231: Toast.makeText(this, "Can't save state: too long", Toast.LENGTH_SHORT).show(); On 2017/02/01 20:24:43, boliu wrote: > maybe include the actual length? round to kb maybe? Done.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2663413002/#ps40001 (title: "Added the actual legth to the toast message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485999582083440,
"parent_rev": "6c8ec587ec4cf305eafb23d136dd4a34fd825730", "commit_rev":
"c58f0323797ee837ccc917c8e31886a0fcbf712e"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485999582083440,
"parent_rev": "6c8ec587ec4cf305eafb23d136dd4a34fd825730", "commit_rev":
"c58f0323797ee837ccc917c8e31886a0fcbf712e"}
Message was sent while issue was closed.
Description was changed from ========== [WebView shell] Discard very long WebView states WebView serializes its state into Android's Bundle so it can restore it through activity recycle. Since N Android has inforced the limit for the size of the bundle, at the same time the length of the serialized state has grew beyond this limit. This CL puts a temorary fix to this problem into SystemWebViewShell only. The states longer than 300k are now dropped by the shell. BUG=686607 ========== to ========== [WebView shell] Discard very long WebView states WebView serializes its state into Android's Bundle so it can restore it through activity recycle. Since N Android has inforced the limit for the size of the bundle, at the same time the length of the serialized state has grew beyond this limit. This CL puts a temorary fix to this problem into SystemWebViewShell only. The states longer than 300k are now dropped by the shell. BUG=686607 Review-Url: https://codereview.chromium.org/2663413002 Cr-Commit-Position: refs/heads/master@{#447681} Committed: https://chromium.googlesource.com/chromium/src/+/c58f0323797ee837ccc917c8e318... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c58f0323797ee837ccc917c8e318... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
