Android WebView: Support data URLs as site URLs (possible test issue) |
||
Issue descriptionIn the fix for issue 863069 , https://chromium-review.googlesource.com/c/chromium/src/+/1150767 changes the site URL of data URLs from "data:" to the full data URL so that data URLs from different mutually untrusted sources don't share a process with each other (which matters for Site Isolation). Unfortunately, this seems to be causing problems two multiple Android WebView tests, which we'll need to investigate for enabling Site Isolation on those platforms. Albert confirmed that my change to SiteInstanceImpl did cause these tests to fail locally, but we're not sure if it's a product problem or just a test framework issue. Example failures: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/53608 webview_instrumentation_test_apk org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeWithTwoViews__multiprocess_mode org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeViewportTagWithTwoViews__multiprocess_mode The tests seem to be timing out: java.util.concurrent.TimeoutException: waitForCallback timed out! at org.chromium.base.test.util.CallbackHelper.waitForCallback(CallbackHelper.java:191) at org.chromium.base.test.util.CallbackHelper.waitForCallback(CallbackHelper.java:227) at org.chromium.android_webview.test.AwSettingsTest$AwSettingsLoadWithOverviewModeTestHelper.doEnsureSettingHasValue(AwSettingsTest.java:1318) at org.chromium.android_webview.test.AwSettingsTest$AwSettingsLoadWithOverviewModeTestHelper.doEnsureSettingHasValue(AwSettingsTest.java:1276) at org.chromium.android_webview.test.AwSettingsTest$AwSettingsTestHelper.ensureSettingHasValue(AwSettingsTest.java:167) at org.chromium.android_webview.test.AwSettingsTest$AwSettingsTestHelper.ensureSettingHasAlteredValue(AwSettingsTest.java:115) at org.chromium.android_webview.test.AwSettingsTest.runPerViewSettingsTest(AwSettingsTest.java:3184) at org.chromium.android_webview.test.AwSettingsTest.testLoadWithOverviewModeViewportTagWithTwoViews(AwSettingsTest.java:2697) at java.lang.reflect.Method.invoke(Native Method) We'd like to understand why having process swaps for data URLs breaks the tests, so that we can avoid making the behavior dependent on whether Site Isolation is enabled.
,
Aug 7
Was looking at this in the morning. There appears to be at least 3 separate problems. 1) the one I mentioned in #1 2) test is too optimistic, need to turn one of the checks into a polling check because it can take awhile for page scale to converge after a load 3) appears to simply be broken behavior, the expected page scale never happens I have 1) and 2) fixed locally right now, but test only passes sometimes. The setting being tested is initialize_at_minimum_page_scale. By that name, presumably it needs to be set before the first page load. And I'm guessing this isn't guaranteed on a cross-site navigation. Sound plausible? Webview overrides webpreferences here: https://cs.chromium.org/chromium/src/android_webview/browser/aw_content_browser_client.cc?rcl=9c14a0ff16764dd001b3025d464f51e9c656465f&l=515
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5345abb27d29f2218e97158206ab23883475c11a commit 5345abb27d29f2218e97158206ab23883475c11a Author: Bo Liu <boliu@chromium.org> Date: Wed Aug 08 15:57:11 2018 aw: Sync scroll state on renderer swap Current behavior is to push browser's scroll state down to the new renderer and a renderer swap, but only under some sequence of events. Pushing from browser to renderer is probably wrong; navigating to a new page generally resets scroll back to top of the page. This CL changes the code to sync renderer scroll state to browser on renderer swap, and does this in all cases. Bug: 871538 Change-Id: Ia887695760b1cce4e3f75cb1ef7091a601e4a556 Reviewed-on: https://chromium-review.googlesource.com/1166150 Reviewed-by: Changwan Ryu <changwan@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#581578} [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/android_webview/browser/browser_view_renderer.cc [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/android_webview/browser/browser_view_renderer.h [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/content/browser/android/synchronous_compositor_host.cc [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/content/browser/android/synchronous_compositor_host.h [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/content/public/browser/android/synchronous_compositor.h [modify] https://crrev.com/5345abb27d29f2218e97158206ab23883475c11a/content/public/test/test_synchronous_compositor_android.h
,
Aug 8
Ah, thanks for tracking that down! wjmaclean@, can you take a look at Bo's findings about page scale in comments 1-2 to confirm? |
||
►
Sign in to add a comment |
||
Comment 1 by boliu@chromium.org
, Aug 6