The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14c2b73e69026eeafcd476fb44460f0e9acc44be

commit 14c2b73e69026eeafcd476fb44460f0e9acc44be
Author: Nicolas Dossou-gbete <dgn@chromium.org>
Date: Mon Oct 23 14:17:33 2017

Disable WebSettingsTest#testAllowMixedMode

Disabling the test so that CTS can be updated and rolled to
account for the new behaviour.

TBR=torne@chromium.org

Bug:  777393 ,412058
Change-Id: I01d9e1de86551e4039535c313a0c5b46c64afe69
Reviewed-on: https://chromium-review.googlesource.com/733083
Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510784}
[modify] https://crrev.com/14c2b73e69026eeafcd476fb44460f0e9acc44be/android_webview/tools/cts_config/expected_failure_on_bot.json

Tim, would you be able to take a look at CTS and see if there is any feasible way to get the test webserver there to support alternate hostnames, given that we can't add chromium command line flags on user builds?

We can't release 64 with this still broken, and ideally we need a CTS fix ASAP so we can get it rolled into an update, so this is high priority.

If we can't find a relatively straightforward way to fix CTS here we might need to consider other options, but I'd rather not.

Actually, marking this as a beta blocker - we should decide what we're going to do here ideally before the branch point near the end of November.

mkwst's CL changed an upstream test. I believe it added in a command line to map real address to localhost: https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java?rcl=62cddd7214b2139a72b0f8782b649e16a8bf375e&l=68

Setting command line is probably not appropriate for CTS though :/

timvolodine@,

This is marked as M64 RBB. Could we have update on this?

Thanks!

OK, did some initial digging, to summarize:

The failing test is android.webkit.cts.WebSettingsTest#testAllowMixedMode [1], which uses both CtsTestWebserver and TestWebServer [2]. Both test servers create a *local* http/https server instance (with "//localhost:.." as address).

In chromium the patch from mkwst@ [3] changed //localhost addresses to *not* be treated as mixed content anymore. The testAllowMixedMode relied on this assumption and hence is broken now.

As mentioned above and elsewhere the chromium implementation for AwSettingsTest.testAllowMixedMode (which is similar to the CTS test), was modified, in particular it uses a commandline switch [4] for the test. However it looks like we cannot use that approach for CTS. (Haven't seen any support/usage for that).

mkwst@:
Is there is a way to formulate the "mixed" test without modification to the command line? that would obviously simplify the issue..


[1] http://androidxref.com/8.0.0_r4/xref/cts/tests/tests/webkit/src/android/webkit/cts/WebSettingsTest.java#943

[2] http://androidxref.com/8.0.0_r4/xref/cts/libs/testserver/src/android/webkit/cts/

[3] https://chromium-review.googlesource.com/702277

[4] https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java?type=cs&q=AwSettingsTest&l=68

The most straightforward thing would be if there's some way to configure the CTS environment to map some other hostname to 127.0.0.1, but I'm not sure that there's any such thing :/

timvolodine@: I think your analysis is correct. The patch in question did two things:

1.  It taught the `TestWebServer` to generate URLs with hostnames other than `localhost`.

2.  It taught the test harness to resolve all hostnames to the loopback address.

As an aside, this capability is pretty important for layout tests in general. I'm a little surprised that none of the CTS tests try to talk to any server other than `localhost`. :(

>   Is there is a way to formulate the "mixed" test without modification to the command line? that would obviously simplify the issue..

If you can come up with a hostname that Android resolves to loopback addresses that isn't covered by https://cs.chromium.org/chromium/src/net/base/url_util.cc?rcl=eb9844b217376646d464ebe3ea88e13e67d1a46f&l=427, we could use it! Alternatively, we could add some sort of terrible API that opted-into `localhost` not being a secure context?

Neither of those options sounds particularly compelling. :/

Thanks for the replies!

It appears "ip6-localhost" [1] instead of "localhost" would qualify as a replacement which would make the CTS test pass (just checked). Although it does sound potentially a bit 'hacky' as this might just be an oversight by the current chrome implementation (?)..

[1]
bullhead:/ # cat etc/hosts
127.0.0.1       localhost
::1             ip6-localhost
bullhead:/ #

"hacky" seems fine if it works! :) I don't intend to add new loopback names to Chrome (and would prefer that we get rid of things like `localhost6`, honestly).

There's not any actual spec that says that ipv6-localhost or localhost6 actually mean anything in particular, is there?

My one concern here would be that maybe they do not, in fact, exist on all android devices, since not everyone's /etc/hosts is guaranteed to be the same..

yeah, well, it does look like the "ip6-localhost" is included in the etc/hosts file since Android M:
http://androidxref.com/6.0.0_r1/xref/system/core/rootdir/etc/hosts#2

Well, that doesn't guarantee it's there on every device, but it makes it much more likely.

Some options that come to mind here at this point (all not ideal):
 - use ip6-localhost
 - add a setting to WebView's WebSettings to allow disabling/enabling this behavior
 - remove the CTS test (? we have this test in chromium; not sure if this is an option though, we probably still want to ensure this behavior is enforced if people tweak webview impl etc..)

Haven't found any CTS url redirection examples yet, except for some renaming trickery (rename localhost to 127.0.0.1) in [1]. Also reached out to the CTS team to check if they know anything about url redirection (torne cc'ed).

[1] http://androidxref.com/8.0.0_r4/xref/cts/tests/tests/webkit/src/android/webkit/cts/CookieManagerTest.java#456

Adding something to WebSettings isn't a possible solution - this has to work for existing Android OS versions that don't have any new APIs we might add. (also it doesn't really make sense to add a way to control this behaviour; nobody is going to want it to be off other than this test).

M64 Beta- coming in 2 weeks, any update on this?

Haven't heard from the CTS guys.. think the immediate options are to use the ipv6-localhost or disable the test altogether.

Reminder that M64 Beta release is nearing, and this is marked as a blocker.  Please take a look soon.

I don't want to disable the localhost change because it's completely reasonable and I think it's extremely unlikely that it will break any applications. The only reason it breaks CTS is because we're conducting a *negative* test that ensures that something doesn't work when the appropriate setting is set; apps are not going to be relying on http://localhost failing to load when mixed mode is disabled.

I'm moving this to RBS because while we really do need to fix CTS ASAP, it doesn't need to block beta if we aren't going to revert the chromium change.

Tim, this really needs to be fixed ASAP though. I think I'd rather we just remove the relevant part of the test rather than use ipv6-localhost which seems like a fragile and gross hack, but can you chase the CTS owners about confirming whether or not there is a way to actually solve this properly first? We also need to talk to them about how we get this CTS fix released, because it's going to start failing for vendors shortly after M64 goes to stable and we need to have a response for them ready.

Yeah, I think we're just going to have to disable this test per the internal thread about CTS.

Friendly ping to get an update on this issue as it is marked as stable blocker.
Thanks..!

timvolodine, can we get an update here?  Please note that M64 Stable is nearing, and this needs to be looked at soon.

Tim, have you prepared the CL to remove this test from CTS? We need to get that landed ASAP, and talk to the CTS owners about how this gets released as soon as possible to the most vendors, so that we minimise the number of bugs we get filed against us about this test failing.

Ping!

Filed https://b.corp.google.com/issues/71852380 for internal reference and uploaded ag/3444942

This seems to be fixed. Please close if that's the case.

Actually, an update for reference:
Thanks to Torne's idea we have been able to fix the test (instead of removing it) yeey!

Corresponding buganizer bug https://b.corp.google.com/issues/71992132, corresponding patch https://googleplex-android-review.git.corp.google.com/c/platform/cts/+/3461793

This no longer needs to block the release as we have a solution.

Tim, you still need to communicate with the CTS owners about how this can be released in a CTS update - we need to let them know that there will be a webview release going out that breaks the old version of the test.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-64; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-64 label, otherwise remove Merge-TBD label. Thanks.

No merge needed, changes are in Android.

Verified, passing on the internal CTS builder now. (see https://b.corp.google.com/issues/71992132)

Hello torne,

In what version of chrome has the fix landed?

No merge needed, changes are in Android. 
what does this mean?

for me , CTS is failing in M64 version of chrome

Thanks

The CTS test was changed so that it would pass. There is no fix in the chrome side; chrome's behaviour is correct already. The new version of CTS probably has not been released yet, but the change is landed in the CTS development branch.