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

In SystemJobService, it can expect null, and able to tolerance null from WorkManagerImpl.getInstance, due to auto-backup reason.

@Override
public void onCreate() {
super.onCreate();
mWorkManagerImpl = WorkManagerImpl.getInstance(getApplicationContext());
if (mWorkManagerImpl == null) {
// This can occur if...
// 1. The app is performing an auto-backup. Prior to O, JobScheduler could erroneously
// try to send commands to JobService in this state (

b/32180780

). Since neither
// Application#onCreate nor ContentProviders have run, WorkManager won't be
// initialized. In this case, we should ignore all JobScheduler commands and tell it
// to retry.
But, current WorkManagerImpl.getInstance(Context context) implementation is that, it will never return null but throw IllegalStateException.

Isn't there's conflict among these 2 SystemJobService.onCreate logic, and WorkManagerImpl.getInstance logic?

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

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

commit e14104eb7008f82edd50b37e16bd119b307e0554
Author: Sumir Kataria <sumir@google.com>
Date: Mon Jun 24 10:50:57 2019

Fix for JobScheduler calls when backup is in progress.

The original fix (ag/Ia1604b7c4b98f6b31869e1161a0c1efce7fe46f2) relied
on a null-check, but the new version of WorkManager.getInstance(Context)
with the Context param doesn't return null; it throws an Exception. This
CL updates the three places where we explicitly check for the return value
internally:

1. SystemJobService (the main place where this would manifest)
2. RescheduleReceiver
3. WorkManagerTestInitHelper (for consistency)

Fixes: 135858602
Test: Ran existing tests
Change-Id: Ia232869712d0c78fd80e91d0a863b8c50a080400

M work/workmanager-testing/src/main/java/androidx/work/testing/WorkManagerTestInitHelper.java
M work/workmanager/src/main/java/androidx/work/impl/background/systemalarm/RescheduleReceiver.java
M work/workmanager/src/main/java/androidx/work/impl/background/systemjob/SystemJobService.java

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

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

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

Thanks you so much for the speedy fixed!

May I know when will it be ready for production? As, currently, this IllegalStateException is impacting our users.

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.

Hello, please look for this later in the week. If you want to work around this bug specifically and don't need on-demand initialization, you can use the old (deprecated) WorkManager.getInstance() method for now.

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

Marked as fixed.

Hello. Thanks for respond. I surely will keep a pair of eye on the next release :)

Yes. We don't need on-demand initialization. May I know how does using old (deprecated) WorkManager.getInstance() in our app code, able to bypass this problem? Isn't the crash is origin from library internal SystemJobService?

We once thinking to bypass this problem, by switching back to Version 2.0.1 (The version just before WorkManager.getInstance(Context) is introduced). However, we notice Room version is being changed in between. Downgrading WorkManager version, might cause additional SQLite problem.