Skip to content

Commit

Permalink
Revert "[WebLayer] Fix relro sharing in GPU and load native lib in th…
Browse files Browse the repository at this point in the history
…e background"

This reverts commit f63587a.

Reason for revert: Breaking tests on Lollipop: crbug.com/1157668

Original change's description:
> [WebLayer] Fix relro sharing in GPU and load native lib in the background
>
> WebLayer relro sharing in the browser process has likely always been
> broken in WebView compatibility mode because the ClassLoader created
> does not have a class loader namespace, see here: https://cs.android.com/android/platform/superproject/+/master:frameworks/base/native/webview/loader/loader.cpp;l=129-132;drc=master
> Unfortunately, this is difficult to fix since sharing relro in WebLayer
> when it is loaded in the same process as WebView causes crashes. This
> will be investigated more in a follow up.
>
> Relro sharing was never implemented for the WebLayer GPU process, and
> this change adds support to do that.
>
> In addition, library loading is kicked off on a separate thread much
> earlier in startup, which allows it to finish before it is needed on the
> UI thread.
>
> Pinpoint runs:
> startup.mobile: https://pinpoint-dot-chromeperf.appspot.com/job/169ab7ef520000
> -18.4% messageloop_start_time
> -3.4% navigation_commit_time
> -1.8% first_contentful_paint_time
>
> weblayer_startup: https://pinpoint-dot-chromeperf.appspot.com/job/17959d48d20000
> -6.4% weblayer_startup_wall_time
>
> system_health.memory_mobile: https://pinpoint-dot-chromeperf.appspot.com/job/16d61b37520000
> -10.0% (3.9MiB) all_processes private_dirty_size
> -29.8% (3.7Mib) gpu_process private_dirty_size
>
> Bug: 1146438
> Change-Id: I52292f0472f18397b0a900307649e6f1b54bd5c1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2581007
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Reviewed-by: Richard Coles <torne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#835580}

TBR=torne@chromium.org,cduvall@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: I0cb581cebbd8e6f0ba315d78035b292501da2aa2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1146438, 1157668
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586279
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835947}
  • Loading branch information
clarkduvall authored and Chromium LUCI CQ committed Dec 11, 2020
1 parent 362350d commit d62d84d
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ private void loadLibraryWithCustomLinker(Linker linker, String library) {
}

private void loadWithChromiumLinker(ApplicationInfo appInfo, String library) {
Linker linker = Linker.getInstance(appInfo);
Linker linker = Linker.getInstance();

if (isInZipFile()) {
String sourceDir = appInfo.sourceDir;
Expand Down
14 changes: 3 additions & 11 deletions base/android/java/src/org/chromium/base/library_loader/Linker.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
package org.chromium.base.library_loader;

import android.annotation.SuppressLint;
import android.content.pm.ApplicationInfo;
import android.os.Bundle;
import android.os.Parcel;
import android.os.ParcelFileDescriptor;
Expand Down Expand Up @@ -203,7 +202,7 @@ protected Linker() {}
*
* @return the Linker implementation instance.
*/
public static Linker getInstance(ApplicationInfo info) {
public static Linker getInstance() {
// A non-monochrome APK (such as ChromePublic.apk) can be installed on N+ in these
// circumstances:
// * installing APK manually
Expand All @@ -223,7 +222,8 @@ public static Linker getInstance(ApplicationInfo info) {
if (sSingleton == null) {
// With incremental install, it's important to fall back to the "normal"
// library loading path in order for the libraries to be found.
String appClass = info.className;
String appClass =
ContextUtils.getApplicationContext().getApplicationInfo().className;
boolean isIncrementalInstall =
appClass != null && appClass.contains("incrementalinstall");
if (LibraryLoader.getInstance().useModernLinker() && !isIncrementalInstall) {
Expand All @@ -237,14 +237,6 @@ public static Linker getInstance(ApplicationInfo info) {
}
}

/**
* Version of getInstance() which will use the ApplicationInfo from
* ContextUtils.getApplicationContext().
*/
public static Linker getInstance() {
return getInstance(ContextUtils.getApplicationContext().getApplicationInfo());
}

/**
* Obtain a random base load address at which to place loaded libraries.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ public final class ChildProcessServiceImpl extends IChildProcessService.Stub {

@UsedByReflection("WebLayer")
public static IBinder create(Service service, Context appContext, Context remoteContext) {
WebLayerImpl.setLibraryPreloader(
remoteContext.getPackageName(), remoteContext.getClassLoader());
ClassLoaderContextWrapperFactory.setLightDarkResourceOverrideContext(
remoteContext, remoteContext);
// Wrap the app context so that it can be used to load WebLayer implementation classes.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.library_loader.LibraryProcessType;
import org.chromium.base.library_loader.NativeLibraryPreloader;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.components.browser_ui.contacts_picker.ContactsPickerDialog;
import org.chromium.components.browser_ui.photo_picker.DecoderServiceHost;
Expand Down Expand Up @@ -221,13 +220,6 @@ private void init(IObjectWrapper appContextWrapper, IObjectWrapper remoteContext
notifyWebViewRunningInProcess(remoteContext.getClassLoader());
}

// Load library in the background since it may be expensive.
// TODO(crbug.com/1146438): Look into enabling relro sharing in browser process. It seems to
// crash when WebView is loaded in the same process.
new Thread(() -> {
LibraryLoader.getInstance().loadNowOverrideApplicationContext(remoteContext);
}).start();

Context appContext = minimalInitForContext(
ObjectWrapper.unwrap(appContextWrapper, Context.class), remoteContext);
PackageInfo packageInfo = WebViewFactory.getLoadedPackageInfo();
Expand Down Expand Up @@ -259,6 +251,12 @@ private void init(IObjectWrapper appContextWrapper, IObjectWrapper remoteContext
/*readCommandLine=*/true);
TraceEvent.begin("WebLayer init");

// If a remote context is not provided, the client is an older version that loads the native
// library on the client side.
if (remoteContextWrapper != null) {
loadNativeLibrary(packageInfo.packageName);
}

BuildInfo.setBrowserPackageInfo(packageInfo);
BuildInfo.setFirebaseAppId(
FirebaseConfig.getFirebaseAppIdForPackage(packageInfo.packageName));
Expand All @@ -280,6 +278,10 @@ private void init(IObjectWrapper appContextWrapper, IObjectWrapper remoteContext
DeviceUtils.addDeviceSpecificUserAgentSwitch();
ContentUriUtils.setFileProviderUtil(new FileProviderHelper());

// TODO: Validate that doing this disk IO on the main thread is necessary.
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
LibraryLoader.getInstance().ensureInitialized();
}
GmsBridge.getInstance().setSafeBrowsingHandler();
GmsBridge.getInstance().initializeBuiltInPaymentApps();

Expand Down Expand Up @@ -320,17 +322,6 @@ public boolean supportsVideos() {
TraceEvent.end("WebLayer init");
}

public static void setLibraryPreloader(String packageName, ClassLoader classLoader) {
if (!LibraryLoader.getInstance().isLoadedByZygote()) {
LibraryLoader.getInstance().setNativeLibraryPreloader(new NativeLibraryPreloader() {
@Override
public int loadLibrary(ApplicationInfo info) {
return loadNativeLibrary(packageName, classLoader);
}
});
}
}

@Override
public IBrowserFragment createBrowserFragmentImpl(
IRemoteFragmentClient fragmentClient, IObjectWrapper fragmentArgs) {
Expand Down Expand Up @@ -760,20 +751,20 @@ private static String getLoadedPackageNames(Context appContext) {
}
}

private static int loadNativeLibrary(String packageName, ClassLoader cl) {
private void loadNativeLibrary(String packageName) {
// Loading the library triggers disk access.
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
return WebViewFactory.loadWebViewNativeLibraryFromPackage(packageName, cl);
WebViewFactory.loadWebViewNativeLibraryFromPackage(
packageName, getClass().getClassLoader());
} else {
try {
Method loadNativeLibrary =
WebViewFactory.class.getDeclaredMethod("loadNativeLibrary");
loadNativeLibrary.setAccessible(true);
return (int) loadNativeLibrary.invoke(null);
loadNativeLibrary.invoke(null);
} catch (ReflectiveOperationException e) {
Log.e(TAG, "Failed to load native library.", e);
return 6; // LIBLOAD_FAILED_TO_LOAD_LIBRARY
}
}
}
Expand Down

0 comments on commit d62d84d

Please sign in to comment.