New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 809636 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Work around font preloading bug in Android O framework

Project Member Reported by torne@chromium.org, Feb 6 2018

Issue description

Apps which use the Android font preloading feature introduced in O crash when attempting to use WebView, because the WebView renderer process attempts to look up the preloaded font list belonging to the application, which is an arbitrary app-specific Android resource ID that is unlikely to exist in the WebView implementation APK.

This happens because the package name of the ApplicationInfo object used in ActivityThread.handleBindApplication is the client app's package name, as an implementation detail of external services. The existing ApplicationInfo object doesn't contain metadata fields, so the ActivityThread code fetches a fresh copy from PackageManager using the package name, which results in it getting the app's metadata instead of WebView's.

We're fixing this in the framework for future Android versions in b/70968451 but it's too late to fix this for O and O MR1 which have already been released with this bug.

It's viable to work around this bug using reflection in Application.onCreate, which is called by ActivityThread before the font preloading takes place. This will require accessing several private framework implementation details in order to replace the PackageManager instance that will be used for the query with one that edits out font preloading info from the returned metadata. It's possible that this code has been customised by device vendors, defeating the reflection, but it's unlikely: the required fields/methods are probably not interesting to alter.

This will require adding a custom Application subclass to the standalone WebView APK, as well as altering the existing MonochromeApplication.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 7 2018

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

commit d1dbf24be0a553ae48c106760de925506fa3e488
Author: Torne (Richard Coles) <torne@google.com>
Date: Wed Feb 07 18:48:30 2018

Work around font preloading bug in Android O framework.

Use reflection during Application.onCreate to wrap the package manager
and strip out font preloading information when the query originates in a
Chromium isolated process. This prevents the framework from crashing due
to a bug in the case where an application that uses WebView has
preloaded fonts specified in its own manifest.

Bug:  809636 
Change-Id: Ife1d647f1dfd096b79775f8da2249661ac67e958
Reviewed-on: https://chromium-review.googlesource.com/905106
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535073}
[modify] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/android_webview/apk/java/AndroidManifest.xml
[modify] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/android_webview/glue/BUILD.gn
[add] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/android_webview/glue/java/src/com/android/webview/chromium/FontPreloadingWorkaround.java
[add] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/android_webview/glue/java/src/com/android/webview/chromium/WebViewApplication.java
[modify] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/chrome/android/java/DEPS
[modify] https://crrev.com/d1dbf24be0a553ae48c106760de925506fa3e488/chrome/android/java/src/org/chromium/chrome/browser/MonochromeApplication.java

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 9 2018

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

commit 4a16a1c9a5857d071d628fe95924f12eb701f326
Author: Torne (Richard Coles) <torne@google.com>
Date: Fri Feb 09 17:27:37 2018

Improve font preloading workaround.

Instead of looking for a specific method call in IPackageManager, just
apply the filtering to any method that returns ApplicationInfo. This
makes it a bit simpler and more robust against changes in this
non-public class. The current AOSP IPackageManager only has one method
that returns ApplicationInfo and it's the method we were checking for,
so this isn't expanding the scope of the workaround unless the framework
has been customised. It's also not necessary to check for GET_META_DATA
since if that wasn't used, the metaData field will simply be null and
the existing check will cover it.

Bug:  809636 
Change-Id: I207f848f4d399684e63dbc62ade199f0244fdecc
Reviewed-on: https://chromium-review.googlesource.com/909588
Commit-Queue: Richard Coles <torne@chromium.org>
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535740}
[modify] https://crrev.com/4a16a1c9a5857d071d628fe95924f12eb701f326/android_webview/glue/java/src/com/android/webview/chromium/FontPreloadingWorkaround.java

Comment 3 by torne@chromium.org, Feb 9 2018

Labels: Merge-Request-65
Project Member

Comment 4 by sheriffbot@chromium.org, Feb 9 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by cmasso@google.com, Feb 12 2018

Do we still need this in M65 even though we are not dropping M65 prebuilt as of this week?

Comment 6 by torne@chromium.org, Feb 12 2018

This is a workaround for O - it's not needed for P where the underlying framework bug has been fixed. So, I'd like to ship this in M65 if we think that's reasonable, to get it out sooner..

Comment 7 by torne@chromium.org, Feb 13 2018

Cc: cma...@chromium.org
Labels: -Hotlist-Merge-Review -M-65 -Merge-Review-65 M-66
Actually, since the major app reporting this issue has already fixed it on their end and in general the problem should be completely obvious in app testing, it's probably no rush to merge it; 66 is fine. The reflection should be safe but it's probably not worth the risk anyhow. Thanks anyway!

Comment 8 by torne@chromium.org, Feb 13 2018

Status: Fixed (was: Started)

Comment 9 by torne@chromium.org, Feb 20 2018

Status: Started (was: Fixed)
This broke the webview stub since the stub's manifest was trying to load WebViewApplication, which doesn't exist in the donor monochrome apk. Fix in CQ.
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/32c6783772ee02341c6a058e9060a48952f32017

commit 32c6783772ee02341c6a058e9060a48952f32017
Author: Torne (Richard Coles) <torne@google.com>
Date: Tue Feb 20 17:38:43 2018

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 20 2018

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

commit 12efe1d50388191d4acf7711a66cf8ba3311ee74
Author: Torne (Richard Coles) <torne@google.com>
Date: Tue Feb 20 17:50:25 2018

Make the webview application class name a variable.

The WebView stub needs to be able to override this to Monochrome's
application class name.

Bug:  809636 
Change-Id: Icbc68f84ce76a8cb2d3ed8ec11d0cca0e00e33e3
Reviewed-on: https://chromium-review.googlesource.com/926724
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537812}
[modify] https://crrev.com/12efe1d50388191d4acf7711a66cf8ba3311ee74/android_webview/apk/java/AndroidManifest.xml

Comment 12 by torne@chromium.org, Feb 20 2018

Status: Fixed (was: Started)
Tested locally that this fixes the stub, so all sorted.

Sign in to add a comment