Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
663543003:
Fix low-memory devices crashing on Chrome start up during UMA recodring (Closed)
|
Created:
6 years, 2 months ago by petrcermak Modified:
6 years, 1 month ago CC:
chromium-reviews, erikwright+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix for a problem on low-memory devices caused by https://codereview.chromium.org/643803003.
This patch prevents low-memory devices from crashing on Chrome start up.
Instead of relying on the Linker.sInBrowserProcess field, which can have
an incorrect value on these devices, the mmap check (whether the device supports
loading libraries directly from APK files) is performed inside
LibraryLoader.onNativeInitializationComplete, which is guaranteed to be called
only in the process thread.
In addition, two extra possible values are added to the UMA histogram:
Unknown (the mmap check was not carried out) and Successful (a library was
successfully loaded directly from the APK).
Finally, this patch removes the deprecated
LibraryLoader.onNativeInitializationComplete() method (with no arguments), which
was replaced by LibraryLoader.onNativeInitializationComplete(Context context) in
https://codereview.chromium.org/643923004/.
BUG=423699, 390618
Committed: https://crrev.com/23b7f2397e7e374eabaf0c3087fc960f4379c21f
Cr-Commit-Position: refs/heads/master@{#301342}
Patch Set 1 #
Total comments: 43
Patch Set 2 : Large update for review feedback #
Total comments: 16
Patch Set 3 : Update for review feedback #Patch Set 4 : Removed LibraryLoader.onNativeInitializationComplete() with no arguments #
Total comments: 4
Patch Set 5 : Final update for review feedback #Patch Set 6 : Simplify Java-C++ enum sharing and fix WebView dependency #
Total comments: 2
Patch Set 7 : Fix recipe trybots failures #Messages
Total messages: 41 (13 generated)
Hi, Please have a look at my CL. rmcilroy: base isherman: tools everybody else: FYI Thanks, Petr https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:63: public static final int LIBRARY_LOAD_FROM_APK_UNKNOWN = 2; I'm not completely satisfied with the way this support flag is passed from Java to C++ (for UMA) here. I can think of four possible solutions: 1. Use two boolean flags (e.g. libraryLoadFromApkChecked and libraryLoadFromApkSupported). While this is in line with the other UMA flags (see library_loader_hooks.cc), it is not very elegant (one variable effectively says whether the other one has any meaning) and scalable (it is likely that there will be more options in the future). 2. Use an int. The main problem with this approach is that we now need to keep three definitions in sync: histograms.xml:Supported, library_loader_hooks.cc:LibraryLoadFromApkSupportCode, and LibraryLoader.java:LIBRARY_LOAD_FROM_APK_*. However, multiple definitions are not avoided by the other solutions either. 3. Use an enum. This would be an ideal choice from a software design point of view. However, it would require adding a lot of boilerplate for transferring an enum between the two languages. 4. Use a string (e.g. "SUP", "NOT", and "UNK"). This would require almost no boilerplate at all. Moreover, it is somewhat less error-prone (from a human point of view) than the int solution because the values have clear meanings. On the other hand, this could introduce many problems associated with strings (encoding, typos, ...). Given all of this, I decided to use ints (option 2). However, I'm open to any other suggestions.
https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:54: // The loader was unable to determine whether the funcionality is supported. nit: "funcionality" -> "functionality" https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:55: UNKNOWN = 2, nit: I'd recommend moving this to be the first item in the enum, in case you ever add more values. https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:2711: - enum="BooleanSupported"> Normally, I'd encourage you to mark the old histogram as <obsolete> rather than removing the description. However, since the histogram was only live for a couple of days in this case, I suppose it's fine either way.
Sorry, histograms LGTM with those nits addressed.
Some lack-of-sleep powered nit picky points. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:186: } Can you set zipfile to null in the 'else', rather defaulting it, it can often make it easier to set break points if values are explicitly set. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:193: Linker.loadLibraryInZipFile(zipfile, library); is (zipfile != null) the right test? Should it be if (Linker.isInZipFile()) [as the golden source] ... unless isInZipFile is really slow, then zipfile works as an OK proxy. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:205: if (!isLoaded) { Might be 1% more readable if you swapped round the function of 'isLoaded'. E.g. call it loadFailed & default it to true. Then you can say if (loadFailed) instead of (!isLoaded) which is (a tiny bit) clearer. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:210: } These lines are duplicated above, should they be moved into a function? The try catch could return true or false, leaving the linker.disabling logic here. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:331: } Might be more readable to swap the if/else bodies and remove the '!'? https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:368: Now that you've started using enums are the first two parameters here really still bools? :) https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; If asserts vanish in release code (?not sure about that?) if might be worth also returning UNKNOWN if apkFile is null. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:58: }; You should probably have the enums named the same on both sides, in case a future developer greps for them. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; Definitely replace the '0' with NOT_SUPPORTED enum. But, as enums are cardinal numbers not ordinal numbers (i.e. they're more like phone numbers that house numbers!) doing less than or greater than checks can be risky when things change in the future. It might be worth considering having a switch statement here with all the valid entries as fall-through cases and the default entry setting the result to UNKNOWN.
https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:63: public static final int LIBRARY_LOAD_FROM_APK_UNKNOWN = 2; On 2014/10/16 18:48:52, petrcermak wrote: > I'm not completely satisfied with the way this support flag is passed from Java > to C++ (for UMA) here. I can think of four possible solutions: > > 1. Use two boolean flags (e.g. libraryLoadFromApkChecked and > libraryLoadFromApkSupported). While this is in line with the other UMA flags > (see library_loader_hooks.cc), it is not very elegant (one variable effectively > says whether the other one has any meaning) and scalable (it is likely that > there will be more options in the future). > > 2. Use an int. The main problem with this approach is that we now need to keep > three definitions in sync: histograms.xml:Supported, > library_loader_hooks.cc:LibraryLoadFromApkSupportCode, and > LibraryLoader.java:LIBRARY_LOAD_FROM_APK_*. However, multiple definitions are > not avoided by the other solutions either. > > 3. Use an enum. This would be an ideal choice from a software design point of > view. However, it would require adding a lot of boilerplate for transferring an > enum between the two languages. > > 4. Use a string (e.g. "SUP", "NOT", and "UNK"). This would require almost no > boilerplate at all. Moreover, it is somewhat less error-prone (from a human > point of view) than the int solution because the values have clear meanings. On > the other hand, this could introduce many problems associated with strings > (encoding, typos, ...). > > Given all of this, I decided to use ints (option 2). However, I'm open to any > other suggestions. The JNI generator can help here. If you look at MemoryPressureList.template (https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...) you can see an example of how to create a Java side int constants which keep in sync with the c++ side. I would follow this model. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:205: if (!isLoaded) { On 2014/10/17 08:23:24, picksi1 wrote: > Might be 1% more readable if you swapped round the function of 'isLoaded'. E.g. > call it loadFailed & default it to true. Then you can say if (loadFailed) > instead of (!isLoaded) which is (a tiny bit) clearer. I would prefer we do these kind of changes in a followup refactoring CL (which this class most definitely needs) rather than in this change since Petr isn't changed this code currently. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:210: } On 2014/10/17 08:23:24, picksi1 wrote: > These lines are duplicated above, should they be moved into a function? The try > catch could return true or false, leaving the linker.disabling logic here. Ditto. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:324: final int libraryLoadFromApkSupport; Nit - we don't usually use final on locals. Personally I'm fine with this but it might be better to avoid it for consistancy. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; On 2014/10/17 08:23:24, picksi1 wrote: > If asserts vanish in release code (?not sure about that?) if might be worth also > returning UNKNOWN if apkFile is null. Yes asserts vanish in release mode (Java assertions also need to be explicitly turned on on the device on Android in order to trigger even on debug builds). Personally I think the assert here is fine, but if you are going to add UNKNOWN here then remove the if/else in libraryloader and state that this function supports a null arg in the Javadoc. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; On 2014/10/17 08:23:24, picksi1 wrote: > Definitely replace the '0' with NOT_SUPPORTED enum. But, as enums are cardinal > numbers not ordinal numbers (i.e. they're more like phone numbers that house > numbers!) doing less than or greater than checks can be risky when things change > in the future. It might be worth considering having a switch statement here with > all the valid entries as fall-through cases and the default entry setting the > result to UNKNOWN. I actually think that might be more error prone since anyone adding an Enum needs to remember to add an entry to the switch statement otherwise it will get overwritten to zero. I think the check with MAX_LIBRARY... is fine - its quite common in the codebase (at least in V8). I agree with making unknown zero though.
https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; If we ditched the default entry in the case statement, the compiler would complain about unhandled enums... but if this is a common pattern, I won't complain. :) FWIW Something I sometimes do is add 2 extra enums that cover the bounds, setting them equal to the relevent Enums e.g. NOT_SUPPORTED, SUPPORTED, UNKNOWN, FIRST_VALID_APK_RESULT = NOT_SUPPORTED, LAST_VALID_APK_RESULT = UNKNOWN This does rely on future devs updating the enum, but it makes the code explicit and more readable where these are used to do the bounds check. e.g. if (( library_load_from_apk_support < FIRST_VALID_APK_RESULT ) || ( library_load_from_apk_support > LAST_VALID_APK_RESULT))
https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:334: libraryLoadFromApkSupport); Actually, I'm just about to turn on loading form the APK for 64-bit. Could we add a seperate enum for LIBRARY_LOADED_FROM_APK and return this instead if Linker.loadLibraryInZipFile is true and succeeds. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; On 2014/10/17 09:23:51, picksi1 wrote: > If we ditched the default entry in the case statement, the compiler would > complain about unhandled enums... but if this is a common pattern, I won't > complain. :) > > > FWIW Something I sometimes do is add 2 extra enums that cover the bounds, > setting them equal to the relevent Enums e.g. > > NOT_SUPPORTED, > SUPPORTED, > UNKNOWN, > FIRST_VALID_APK_RESULT = NOT_SUPPORTED, > LAST_VALID_APK_RESULT = UNKNOWN > > This does rely on future devs updating the enum, but it makes the code explicit > and more readable where these are used to do the bounds check. e.g. > > if (( library_load_from_apk_support < FIRST_VALID_APK_RESULT ) || > ( library_load_from_apk_support > LAST_VALID_APK_RESULT)) +1 this sounds good. https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:52585: +<enum name="Support" type="int"> If we add LoadedFromApk to this enum then we should probably make the name more specific to the histogram, and drop the "Supported" from the histogram name above.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
petrcermak@chromium.org changed reviewers: + danakj@chromium.org
Thanks for your comments. PTAL. danakj: base rmcilroy: base/android isherman: tools everybody else: FYI Thanks, Petr https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:63: public static final int LIBRARY_LOAD_FROM_APK_UNKNOWN = 2; On 2014/10/17 09:12:59, rmcilroy wrote: > On 2014/10/16 18:48:52, petrcermak wrote: > > I'm not completely satisfied with the way this support flag is passed from > Java > > to C++ (for UMA) here. I can think of four possible solutions: > > > > 1. Use two boolean flags (e.g. libraryLoadFromApkChecked and > > libraryLoadFromApkSupported). While this is in line with the other UMA flags > > (see library_loader_hooks.cc), it is not very elegant (one variable > effectively > > says whether the other one has any meaning) and scalable (it is likely that > > there will be more options in the future). > > > > 2. Use an int. The main problem with this approach is that we now need to keep > > three definitions in sync: histograms.xml:Supported, > > library_loader_hooks.cc:LibraryLoadFromApkSupportCode, and > > LibraryLoader.java:LIBRARY_LOAD_FROM_APK_*. However, multiple definitions are > > not avoided by the other solutions either. > > > > 3. Use an enum. This would be an ideal choice from a software design point of > > view. However, it would require adding a lot of boilerplate for transferring > an > > enum between the two languages. > > > > 4. Use a string (e.g. "SUP", "NOT", and "UNK"). This would require almost no > > boilerplate at all. Moreover, it is somewhat less error-prone (from a human > > point of view) than the int solution because the values have clear meanings. > On > > the other hand, this could introduce many problems associated with strings > > (encoding, typos, ...). > > > > Given all of this, I decided to use ints (option 2). However, I'm open to any > > other suggestions. > > The JNI generator can help here. If you look at MemoryPressureList.template > (https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...) > you can see an example of how to create a Java side int constants which keep in > sync with the c++ side. I would follow this model. Done. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:186: } On 2014/10/17 08:23:24, picksi1 wrote: > Can you set zipfile to null in the 'else', rather defaulting it, it can often > make it easier to set break points if values are explicitly set. Agree. Also, then the variable could be declared as final (which helps prevent bugs). However, should such changes be done as part of this CL? This whole class (and Linker) needs to go through major refactoring later anyway. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:193: Linker.loadLibraryInZipFile(zipfile, library); On 2014/10/17 08:23:23, picksi1 wrote: > is (zipfile != null) the right test? Should it be if (Linker.isInZipFile()) [as > the golden source] ... unless isInZipFile is really slow, then zipfile works as > an OK proxy. ditto https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:205: if (!isLoaded) { On 2014/10/17 08:23:24, picksi1 wrote: > Might be 1% more readable if you swapped round the function of 'isLoaded'. E.g. > call it loadFailed & default it to true. Then you can say if (loadFailed) > instead of (!isLoaded) which is (a tiny bit) clearer. ditto. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:210: } On 2014/10/17 08:23:24, picksi1 wrote: > These lines are duplicated above, should they be moved into a function? The try > catch could return true or false, leaving the linker.disabling logic here. ditto. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:324: final int libraryLoadFromApkSupport; On 2014/10/17 09:12:59, rmcilroy wrote: > Nit - we don't usually use final on locals. Personally I'm fine with this but it > might be better to avoid it for consistancy. Done (although I've seen it used in Linker.java for example and think it is a good code practice). https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:331: } On 2014/10/17 08:23:24, picksi1 wrote: > Might be more readable to swap the if/else bodies and remove the '!'? I thought about it and I would prefer to keep it this way as the "!= null" case is the "normal" one (so it seems more natural to me to have it first). https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:334: libraryLoadFromApkSupport); On 2014/10/17 09:54:45, rmcilroy wrote: > Actually, I'm just about to turn on loading form the APK for 64-bit. Could we > add a seperate enum for LIBRARY_LOADED_FROM_APK and return this instead if > Linker.loadLibraryInZipFile is true and succeeds. Done. Note that Linker.loadLibraryInZipFile doesn't return true but it throws an exception if the load fails. There is one issue: In the (presumably) unlikely event of first loading a library *not* from the APK and then loading the same library *from* the APK, LIBRARY_LOAD_FROM_APK_SUCCESSFUL will be reported (because the Linker doesn't load a library twice). Is this something we need to take care of? If yes, we need to change the return type of Linker.loadLibraryInZipFile or move the new sLibraryLoadedFromApk flag from the LibraryLoader to the Linker. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:368: On 2014/10/17 08:23:24, picksi1 wrote: > Now that you've started using enums are the first two parameters here really > still bools? :) ditto (as line 181). https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; On 2014/10/17 08:23:24, picksi1 wrote: > If asserts vanish in release code (?not sure about that?) if might be worth also > returning UNKNOWN if apkFile is null. This seems unnecessary because the method is called (from LibraryLoader.recordBrowserProcessHistogram) only if the file name is not null. I added a note to the javadoc that apkFile has to be "not null". https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; On 2014/10/17 09:12:59, rmcilroy wrote: > On 2014/10/17 08:23:24, picksi1 wrote: > > If asserts vanish in release code (?not sure about that?) if might be worth > also > > returning UNKNOWN if apkFile is null. > > Yes asserts vanish in release mode (Java assertions also need to be explicitly > turned on on the device on Android in order to trigger even on debug builds). > Personally I think the assert here is fine, but if you are going to add UNKNOWN > here then remove the if/else in libraryloader and state that this function > supports a null arg in the Javadoc. I would prefer returning a boolean here. If you think that the assert is insufficient, I can make it throw a NullPointerException. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:54: // The loader was unable to determine whether the funcionality is supported. On 2014/10/16 19:00:30, Ilya Sherman wrote: > nit: "funcionality" -> "functionality" Done. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:55: UNKNOWN = 2, On 2014/10/16 19:00:30, Ilya Sherman wrote: > nit: I'd recommend moving this to be the first item in the enum, in case you > ever add more values. Done. It is very likely that more values will indeed be added in the future. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:58: }; On 2014/10/17 08:23:24, picksi1 wrote: > You should probably have the enums named the same on both sides, in case a > future developer greps for them. Done. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; On 2014/10/17 09:12:59, rmcilroy wrote: > On 2014/10/17 08:23:24, picksi1 wrote: > > Definitely replace the '0' with NOT_SUPPORTED enum. But, as enums are cardinal > > numbers not ordinal numbers (i.e. they're more like phone numbers that house > > numbers!) doing less than or greater than checks can be risky when things > change > > in the future. It might be worth considering having a switch statement here > with > > all the valid entries as fall-through cases and the default entry setting the > > result to UNKNOWN. > > I actually think that might be more error prone since anyone adding an Enum > needs to remember to add an entry to the switch statement otherwise it will get > overwritten to zero. I think the check with MAX_LIBRARY... is fine - its quite > common in the codebase (at least in V8). > > I agree with making unknown zero though. Done. https://codereview.chromium.org/663543003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:108: library_load_from_apk_support = UNKNOWN; On 2014/10/17 09:23:51, picksi1 wrote: > If we ditched the default entry in the case statement, the compiler would > complain about unhandled enums... but if this is a common pattern, I won't > complain. :) > > > FWIW Something I sometimes do is add 2 extra enums that cover the bounds, > setting them equal to the relevent Enums e.g. > > NOT_SUPPORTED, > SUPPORTED, > UNKNOWN, > FIRST_VALID_APK_RESULT = NOT_SUPPORTED, > LAST_VALID_APK_RESULT = UNKNOWN > > This does rely on future devs updating the enum, but it makes the code explicit > and more readable where these are used to do the bounds check. e.g. > > if (( library_load_from_apk_support < FIRST_VALID_APK_RESULT ) || > ( library_load_from_apk_support > LAST_VALID_APK_RESULT)) After a discussion with simonb, we concluded that there is no point in checking this since UMA_HISTOGRAM_ENUMERATION checks value range automatically (the only difference being that there won't be an error log message). https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:2711: - enum="BooleanSupported"> On 2014/10/16 19:00:30, Ilya Sherman wrote: > Normally, I'd encourage you to mark the old histogram as <obsolete> rather than > removing the description. However, since the histogram was only live for a > couple of days in this case, I suppose it's fine either way. Acknowledged. https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/663543003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:52585: +<enum name="Support" type="int"> On 2014/10/17 09:54:45, rmcilroy wrote: > If we add LoadedFromApk to this enum then we should probably make the name more > specific to the histogram, and drop the "Supported" from the histogram name > above. I changed the name of the enum to "LibraryLoadFromApkSupport" and the possible values are "Unknown", "Not supported", "Supported", and "Successful". I kept the histogram as "ChromiumAndroidLinker.LibraryLoadFromApkSupport". However, I am not completely happy about this so feel free to suggest other names (for the enum, the histogram, and/or possible values).
Looks good, a couple more comments. Could you also please update the title and description to be more descriptive and mention the fact that you are adding the logging as to whether the library is loaded from APK. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; On 2014/10/17 14:51:28, petrcermak wrote: > On 2014/10/17 09:12:59, rmcilroy wrote: > > On 2014/10/17 08:23:24, picksi1 wrote: > > > If asserts vanish in release code (?not sure about that?) if might be worth > > also > > > returning UNKNOWN if apkFile is null. > > > > Yes asserts vanish in release mode (Java assertions also need to be > explicitly > > turned on on the device on Android in order to trigger even on debug builds). > > Personally I think the assert here is fine, but if you are going to add > UNKNOWN > > here then remove the if/else in libraryloader and state that this function > > supports a null arg in the Javadoc. > > I would prefer returning a boolean here. If you think that the assert is > insufficient, I can make it throw a NullPointerException. I actually think the assert is unnecessary - the native code would fail with a NPE in any case if Null is passed, which I think is clear enough as to what went wrong. I don't object to having the assert though, so I'm fine with this as is. https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn#newcode58 base/BUILD.gn:58: "android/library_loader/library_load_from_apk_support_code.h", "support_code" sounds like it is code to add support for loading from the library. How about "library_load_from_apk_status_codes.h" (with a similar rename for the Java)? https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn#newcode1439 base/BUILD.gn:1439: "android/java/src/org/chromium/base/library_loader/LibraryLoadFromApkSupportCode.template", I think you've forgotten to upload this file ;). https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:56: // One-way switch becomes true when the library is successfully loaded from nit - One-way switch becomes true if the library was loaded from the APK directly. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:58: private static boolean sLibraryLoadedFromApk = false; nit - sLibraryWasLoadedFromApk https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:319: private static void recordBrowserProcessHistogram() { It looks like this is only called from onNativeInitializationComplete(), which itself is only called from NativeInitializationController, which has a mContext field. Could we pass the context to this function in order to avoid storing sApkFileName as a static field? https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:321: int libraryLoadFromApkSupport; nit - libraryLoadFromApkStatus https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:331: } Please pull this out into a separate function which returns the libraryLoadFromApkStatus and just call it on the line below. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:827: * @param apkFile Filename of the APK (not null). this is usually implied (with params which can be null marked as such instead).
Patchset #3 (id:80001) has been deleted
Thanks for your comments. PTAL. https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:831: assert apkFile != null; On 2014/10/20 10:42:39, rmcilroy wrote: > On 2014/10/17 14:51:28, petrcermak wrote: > > On 2014/10/17 09:12:59, rmcilroy wrote: > > > On 2014/10/17 08:23:24, picksi1 wrote: > > > > If asserts vanish in release code (?not sure about that?) if might be > worth > > > also > > > > returning UNKNOWN if apkFile is null. > > > > > > Yes asserts vanish in release mode (Java assertions also need to be > > explicitly > > > turned on on the device on Android in order to trigger even on debug > builds). > > > Personally I think the assert here is fine, but if you are going to add > > UNKNOWN > > > here then remove the if/else in libraryloader and state that this function > > > supports a null arg in the Javadoc. > > > > I would prefer returning a boolean here. If you think that the assert is > > insufficient, I can make it throw a NullPointerException. > > I actually think the assert is unnecessary - the native code would fail with a > NPE in any case if Null is passed, which I think is clear enough as to what went > wrong. I don't object to having the assert though, so I'm fine with this as is. Acknowledged. https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn#newcode58 base/BUILD.gn:58: "android/library_loader/library_load_from_apk_support_code.h", On 2014/10/20 10:42:39, rmcilroy wrote: > "support_code" sounds like it is code to add support for loading from the > library. How about "library_load_from_apk_status_codes.h" (with a similar > rename for the Java)? Done. I changed the histogram too. https://codereview.chromium.org/663543003/diff/60001/base/BUILD.gn#newcode1439 base/BUILD.gn:1439: "android/java/src/org/chromium/base/library_loader/LibraryLoadFromApkSupportCode.template", On 2014/10/20 10:42:39, rmcilroy wrote: > I think you've forgotten to upload this file ;). Done (I'd forgotten the file below as well). https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:56: // One-way switch becomes true when the library is successfully loaded from On 2014/10/20 10:42:39, rmcilroy wrote: > nit - One-way switch becomes true if the library was loaded from the APK > directly. Done. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:58: private static boolean sLibraryLoadedFromApk = false; On 2014/10/20 10:42:39, rmcilroy wrote: > nit - sLibraryWasLoadedFromApk Done. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:319: private static void recordBrowserProcessHistogram() { On 2014/10/20 10:42:39, rmcilroy wrote: > It looks like this is only called from onNativeInitializationComplete(), which > itself is only called from NativeInitializationController, which has a mContext > field. Could we pass the context to this function in order to avoid storing > sApkFileName as a static field? Done. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:321: int libraryLoadFromApkSupport; On 2014/10/20 10:42:39, rmcilroy wrote: > nit - libraryLoadFromApkStatus The variable is not needed now that the code is in a separate function. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:331: } On 2014/10/20 10:42:39, rmcilroy wrote: > Please pull this out into a separate function which returns the > libraryLoadFromApkStatus and just call it on the line below. Done. https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/663543003/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:827: * @param apkFile Filename of the APK (not null). On 2014/10/20 10:42:39, rmcilroy wrote: > this is usually implied (with params which can be null marked as such instead). Removed.
lgtm!
LGTM, thanks Petr. SimonB - do you have any comments before Petr lands this?
FYI: I removed the deprecated LibraryLoader.onNativeInitializationComplete() method with no arguments.
The GYP/GN changes in base LGTM w/ some comments https://codereview.chromium.org/663543003/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/663543003/diff/120001/base/BUILD.gn#newcode1446 base/BUILD.gn:1446: java_cpp_template("base_java_library_load_from_apk_status_codes") { can you order this the same as they appear in the list above? https://codereview.chromium.org/663543003/diff/120001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/663543003/diff/120001/base/base.gyp#newcode1360 base/base.gyp:1360: # GN: //base:base_java_library_load_from_apk_status_codes order the same as the list above
Thanks for your comments. I will now commit the patch. https://codereview.chromium.org/663543003/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/663543003/diff/120001/base/BUILD.gn#newcode1446 base/BUILD.gn:1446: java_cpp_template("base_java_library_load_from_apk_status_codes") { On 2014/10/23 15:12:36, danakj wrote: > can you order this the same as they appear in the list above? Done. https://codereview.chromium.org/663543003/diff/120001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/663543003/diff/120001/base/base.gyp#newcode1360 base/base.gyp:1360: # GN: //base:base_java_library_load_from_apk_status_codes On 2014/10/23 15:12:36, danakj wrote: > order the same as the list above Done.
lgtm
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663543003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
petrcermak@chromium.org changed reviewers: + mkosiba@chromium.org
I made some changes to simplify Java-C++ enum sharing and fix WebView dependency (which was breaking the build on android_aosp). PTAL: mkosiba: android_webview danakj: base (build files) rmcilroy: base/android/library_loader Thanks, Petr
android_webview/ LGTM https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... File base/android/library_loader/library_load_from_apk_status_codes.h (right): https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... base/android/library_loader/library_load_from_apk_status_codes.h:16: LIBRARY_LOAD_FROM_APK_STATUS_CODES_UNKNOWN = 0, FYI: you don't need to explicitly assign the values for the C++->Java enum script to work (it's fine if you want to, just saying you don't have to).
On 2014/10/24 14:17:26, mkosiba wrote: > android_webview/ LGTM > > https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... > File base/android/library_loader/library_load_from_apk_status_codes.h (right): > > https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... > base/android/library_loader/library_load_from_apk_status_codes.h:16: > LIBRARY_LOAD_FROM_APK_STATUS_CODES_UNKNOWN = 0, > FYI: you don't need to explicitly assign the values for the C++->Java enum > script to work (it's fine if you want to, just saying you don't have to). Nice, I didn't know of java_cpp_enum.gypi, that's much better. lgtm, thanks!
The CQ bit was checked by petrcermak@chromium.org
the gyp/gn still lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663543003/200001
Thanks for your comments. https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... File base/android/library_loader/library_load_from_apk_status_codes.h (right): https://codereview.chromium.org/663543003/diff/200001/base/android/library_lo... base/android/library_loader/library_load_from_apk_status_codes.h:16: LIBRARY_LOAD_FROM_APK_STATUS_CODES_UNKNOWN = 0, On 2014/10/24 14:17:26, mkosiba wrote: > FYI: you don't need to explicitly assign the values for the C++->Java enum > script to work (it's fine if you want to, just saying you don't have to). Acknowledged (I kept the explicit numbering to make sure that it is kept in sync with the corresponding histogram in tools/metrics/histograms/histograms.xml).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663543003/220001
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/23b7f2397e7e374eabaf0c3087fc960f4379c21f Cr-Commit-Position: refs/heads/master@{#301342} |
