The main dex list is missing classes loaded by the renderer |
||||
Issue descriptionA number of classes that are loaded by the renderer aren't kept in the main dex list (generated by https://cs.chromium.org/chromium/src/build/android/gyp/main_dex_list.py?q=main_dex_list&sq=package:chromium&l=1). This likely occurs because offending classes are missing an @MainDex annotation, they are erroneously registered during JNI registration, or because they should actually live in common/ since they are used by the renderer. This prevents us from building a minimal main dex that contains only classes used by the renderer so that we can save memory in each renderer process in release builds. We should: 1. Ensure that the main dex file includes all classes loaded by the renderer 2. Introduce a check to ensure that tests will fail when the renderer accesses a class from secondary dex files Note: This only applies to L+ since prior versions require a support library to enable multidex which introduces a big startup regression.
,
Oct 18 2016
Offending classes when building ChromePublic: org.chromium.base.CpuFeatures org.chromium.base.ImportantFileWriterAndroid org.chromium.base.JavaHandlerThread org.chromium.base.PathService org.chromium.base.metrics.StatisticsRecorderAndroid org.chromium.ui.gfx.ViewConfigurationHelper org.chromium.net.GURLUtils org.chromium.net.HttpNegotiateAuthenticator org.chromium.net.ProxyChangeListener org.chromium.net.X509Util org.chromium.ui.base.LocalizationUtils org.chromium.ui.gl.SurfaceTextureListener org.chromium.content_public.common.ResourceRequestBody org.chromium.device.bluetooth.ChromeBluetoothAdapter org.chromium.device.bluetooth.ChromeBluetoothDevice org.chromium.device.bluetooth.ChromeBluetoothRemoteGattCharacteristic org.chromium.device.bluetooth.ChromeBluetoothRemoteGattDescriptor org.chromium.device.bluetooth.ChromeBluetoothRemoteGattService org.chromium.device.gamepad.GamepadList org.chromium.device.sensors.PlatformSensor org.chromium.device.time_zone_monitor.TimeZoneMonitor org.chromium.device.usb.ChromeUsbService org.chromium.media.AudioManagerAndroid org.chromium.media.AudioRecordInput org.chromium.media.MediaPlayerBridge org.chromium.media.MediaPlayerListener org.chromium.media.VideoCapture org.chromium.midi.UsbMidiDeviceAndroid org.chromium.midi.UsbMidiDeviceFactoryAndroid org.chromium.midi.MidiManagerAndroid org.chromium.midi.MidiInputPortAndroid org.chromium.media.ScreenCapture
,
Oct 18 2016
,
Oct 19 2016
It seems that the classes in #2 are required because we complete JNI registration regardless of whether we're in the browser process or not, and the classes in #2 aren't kept on the main dex list when we enable multidex (not reachable from ChromeApplication). So, when we try to register them outside of the browser process (even when we don't need them) Chrome crashes because the classes aren't found. A few possible approaches: 1. Maintain a list of these classes and add -keep rules in main_dex_classes.flags (see https://codereview.chromium.org/2423323003/) 2. Prevent registration of browser-only JNI manually (like https://codereview.chromium.org/2316183002) 3. Refactor JNI registration to be smarter (i.e. don't do JNI registration for webview, only register browser classes when in the browser process) Thoughts? Torne, I cc'ed you because you're involved with bug #512357 and you reviewed agrieve's CL (mentioned above), but feel free to remove/replace yourself if there's someone else interested in this kind of stuff.
,
Oct 24 2016
As I mentioned on that CL, I think we should probably move toward a world where we have three "states" for JNI registration: 1) Register all functions 2) Register only functions used by the renderer 3) Don't register anything Regular clank builds can use 1) and 2) for the browser vs renderer processes respectively, and WebView/Monochrome can use 3). We then don't need to directly test the process type in lots of places, and can probably avoid calling a lot of code paths at all on webview/monochrome.
,
Oct 25 2016
3) is already the case for WebView and Monochrome, right? Here's a short design doc related to this bug: https://docs.google.com/document/d/1rsVf3UKY_2SbrIj-LfGZuOw3_flb4WPTDE5iPF0FVnc/edit#heading=h.fe0km7fob016
,
Oct 26 2016
No. WebView code paths try not to call registration functions, but we're not sure if we actually succeed (due to shared code with chrome maybe calling them), and so we also shortcircuit the actual registration implementations in the JNI generator using a global boolean (yay flags). Monochrome currently just registers everything, unnecessarily. I added some comments to the doc; I'm not sure I like any of your three options.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96c577b130b3317721d525a252bcdc82245d3bc3 commit 96c577b130b3317721d525a252bcdc82245d3bc3 Author: estevenson <estevenson@chromium.org> Date: Thu Oct 27 22:55:40 2016 Add @MainDex to DeviceDisplayInfo. DeviceDisplayInfo is used in the render process and should always be included in the main dex file. BUG= 657024 Review-Url: https://codereview.chromium.org/2445783003 Cr-Commit-Position: refs/heads/master@{#428181} [modify] https://crrev.com/96c577b130b3317721d525a252bcdc82245d3bc3/ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96ce916013bb343950629bef619ae67aed4f1fb9 commit 96ce916013bb343950629bef619ae67aed4f1fb9 Author: estevenson <estevenson@chromium.org> Date: Thu Oct 27 23:40:57 2016 Add @MainDex to MediaCodecUtil. MediaCodecUtil is used in non-browser processes, so it should be kept in the main dex file. This keeps the previously annotated inner classes in the main dex file as well. BUG= 657024 Review-Url: https://codereview.chromium.org/2460713002 Cr-Commit-Position: refs/heads/master@{#428219} [modify] https://crrev.com/96ce916013bb343950629bef619ae67aed4f1fb9/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java
,
Nov 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c11bed35d016206740715e7b990ef870ba62a5a7 commit c11bed35d016206740715e7b990ef870ba62a5a7 Author: estevenson <estevenson@chromium.org> Date: Wed Nov 02 22:26:17 2016 Fix java classes missing @MainDex. Adding @MainDex to these classes will ensure they are kept in the main dex file even when proguard is used. This will allow us to turn on multidex in release. BUG= 657024 Review-Url: https://codereview.chromium.org/2463323003 Cr-Commit-Position: refs/heads/master@{#429420} [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/base/android/java/src/org/chromium/base/ContextUtils.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/base/android/java/src/org/chromium/base/TraceEvent.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/content/public/android/java/src/org/chromium/content/app/ContentApplication.java [modify] https://crrev.com/c11bed35d016206740715e7b990ef870ba62a5a7/content/public/android/java/src/org/chromium/content/app/ContentMain.java
,
Nov 8 2016
Cronet has errors "java.lang.NoClassDefFoundError: org/chromium/base/metrics/StatisticsRecorderAndroid" since 55.0.2868.0. Are you going to add @MainDex to org/chromium/base/metrics/StatisticsRecorderAndroid as well? (more details see internal bug b/32694056)
,
Nov 8 2016
I'll add @MainDex to StatisticsRecorderAndroid with a TODO to remove it once this bug is fixed since it isn't actually needed in the main dex.
,
Nov 8 2016
Thanks! I don't actually know why this split is affecting Cronet now. We didn't have this problem before. Why is this class not found when Cronet tries to do JNI registration? Is it because the class is stripped in proguard? I am not sure if adding the annotation would fix this problem, since Cronet doesn't use build/android/main_dex_classes.flags. Could you explain a bit why Cronet runs into "java.lang.NoClassDefFoundError: org/chromium/base/metrics/StatisticsRecorderAndroid"? Does this only fail on some platforms and not on others? One cronet embedder is seeing ~50 reports per day with this error. Thanks!
,
Dec 16 2016
Closing this issue and moving the JNI registration refactor to issue 674937.
,
Jan 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27392dadd471377c10736953fedbb18f76cbf2e8 commit 27392dadd471377c10736953fedbb18f76cbf2e8 Author: estevenson <estevenson@chromium.org> Date: Mon Jan 30 16:40:15 2017 Add @MainDex annotation to classes used in child processes. These classes are used by render processes during telemetry_perf_unittests on Android. This doesn't cause any problems currently, but is necessary to be fixed for implementing selective JNI registration (https://crbug.com/674937). BUG= 657024 Review-Url: https://codereview.chromium.org/2622303005 Cr-Commit-Position: refs/heads/master@{#446999} [modify] https://crrev.com/27392dadd471377c10736953fedbb18f76cbf2e8/base/android/java/src/org/chromium/base/EarlyTraceEvent.java [modify] https://crrev.com/27392dadd471377c10736953fedbb18f76cbf2e8/base/android/java/src/org/chromium/base/TimeUtils.java |
||||
►
Sign in to add a comment |
||||
Comment 1 by estevenson@chromium.org
, Oct 18 2016