Order should not matter for AssetLoader builder [182196765]

Fixed

Bug

Status Update

No update yet.

Description

nt...@google.com

created issue #1

Mar 8, 2021 09:17PM

I was playing around with AssetLoader and I found that the order of builder arguments matters. I found this extremely surprising, and this is not the normal convention for builders. As an example, here are two pieces of Java code which do different things:

// Code snippet 1...

        final WebViewAssetLoader assetLoader = new WebViewAssetLoader.Builder()

                // Set the domain first and everything works!
                .setDomain("www.fakedomain.com")

                .addPathHandler("/assets/", new WebViewAssetLoader.AssetsPathHandler(this))
                .addPathHandler("/res/", new WebViewAssetLoader.ResourcesPathHandler(this))
                .build();
        mWebView.setWebViewClient(new LocalContentWebViewClient(assetLoader));
        String inAppHtmlUrl = "https://www.fakedomain.com/assets/index.html";
        mWebView.loadUrl(inAppHtmlUrl);

// Code snippet 2...

        final WebViewAssetLoader assetLoader = new WebViewAssetLoader.Builder()
                .addPathHandler("/assets/", new WebViewAssetLoader.AssetsPathHandler(this))
                .addPathHandler("/res/", new WebViewAssetLoader.ResourcesPathHandler(this))

                // But if we set the domain after PathHandlers, WebView tries to load from the internet
                .setDomain("www.fakedomain.com")

                .build();
        mWebView.setWebViewClient(new LocalContentWebViewClient(assetLoader));
        String inAppHtmlUrl = "https://www.fakedomain.com/assets/index.html";
        mWebView.loadUrl(inAppHtmlUrl);

The issue is we dereference the mDomain when addPathHandler is called, but we really should wait until .build() is called. Same problem with mHttpAllowed. See https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:webkit/webkit/src/main/java/androidx/webkit/WebViewAssetLoader.java;l=526;drc=764112d6d736023c63322b262bec9c93e67786ca

Comments

mi...@google.com <mi...@google.com> Mar 9, 2021 05:25PM

Reassigned to ha...@google.com.

ha...@google.com <ha...@google.com> #2Mar 9, 2021 07:03PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

https://android-review.googlesource.com/1123258

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

ha...@google.com <ha...@google.com> #3Mar 9, 2021 07:07PM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

ha...@google.com <ha...@google.com> #4Apr 1, 2021 04:24PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

https://android-review.googlesource.com/1288456

nt...@google.com <nt...@google.com> Apr 2, 2021 08:01PM

Reassigned to nt...@google.com.

nt...@google.com <nt...@google.com> #5Apr 2, 2021 10:52PM

Accepted by nt...@google.com.

I believe https://android-review.googlesource.com/c/platform/frameworks/support/+/1663126 should address this, and includes test coverage for the cases which are relevant (order doesn't matter for setDomain() or setHttpAllowed() but it does matter for multiple addPathHandler() calls).

ap...@google.com <ap...@google.com> #6Apr 5, 2021 02:25PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 3fafde1b853f8c8155e05d6fcea554af0b11d9a6
Author: Nate Fischer <ntfschr@google.com>
Date: Fri Apr 02 12:45:41 2021

AndroidX Webkit: fix ordering issues on AssetLoader.Builder

We observed some WebViewAssetLoader.Builder methods are order-dependent
in a surprising way. This fixes the builder to no longer care about the
ordering of setDomain and setHttpAllowed in relation to addPathHandler
calls. This adds two tests for this case.

There are still good use cases for addPathHandler() calls to be
order-dependent. I noticed this wasn't actually covered by any
integration tests, so I added one to cover this case as well. I updated
addPathHandler's javadoc to clarify that order matters, although this
was already documented on the PathHandler interface javadoc.

Fixes: 182196765
Test: ./gradlew :webkit:webkit:connectedAndroidTest
Relnote: "Fixed a bug where WebViewAssetLoader.Builder methods were
unintentionally order-dependent."
Change-Id: If420d559a21ea624b2a493d09550ed496ffee392

M webkit/webkit/src/androidTest/java/androidx/webkit/WebViewAssetLoaderTest.java
M webkit/webkit/src/main/java/androidx/webkit/WebViewAssetLoader.java

https://android-review.googlesource.com/1663126