Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
684163002:
Library loading fallback. (Closed)
|
Created:
6 years, 1 month ago by petrcermak Modified:
5 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, picksi1, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionLibrary loading fallback.
This patch adds support for copying the Chromium library from the APK file
to a separate file.
This patch is largly based on https://codereview.chromium.org/673093005/.
BUG=390618
Committed: https://crrev.com/51ca0822d0f57cd8f6dd82dd6fe49ab6c14c1b1b
Cr-Commit-Position: refs/heads/master@{#302456}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Update for review feedback #
Total comments: 49
Patch Set 3 : Large update for review feedback #
Total comments: 31
Patch Set 4 : Update for review feedback #
Total comments: 4
Patch Set 5 : Final update for review feedback #Messages
Total messages: 24 (4 generated)
petrcermak@chromium.org changed reviewers: + anton@chromium.org, rmcilroy@chromium.org, simonb@chromium.org
Hi, this is a refactored version of the fallback. Petr
https://codereview.chromium.org/684163002/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/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:206: apkFilename, System.mapLibraryName(library))) { Replace System.mapLibraryName(library) with libFilename https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:430: throw new SecurityException( SecurityException? https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:469: FALLBACK_DIR, Context.MODE_WORLD_READABLE).getPath(); We should look into whether there is an alternative to Context.MODE_WORLD_READABLE https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:512: Log.i(TAG, "Fallback process took " + (after - before) + "ms"); "Building fallback library took .."
Patchset #2 (id:20001) has been deleted
Thanks for your comments. Petr https://codereview.chromium.org/684163002/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/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:206: apkFilename, System.mapLibraryName(library))) { On 2014/10/29 13:35:35, Anton wrote: > Replace System.mapLibraryName(library) with libFilename Done. https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:430: throw new SecurityException( On 2014/10/29 13:35:35, Anton wrote: > SecurityException? Done (no idea how this happened). https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:469: FALLBACK_DIR, Context.MODE_WORLD_READABLE).getPath(); On 2014/10/29 13:35:35, Anton wrote: > We should look into whether there is an alternative to > Context.MODE_WORLD_READABLE It doesn't look like there are any other options. I doubt that ContentProvider, BroadcastReceiver or Service (which are suggested on http://developer.android.com/reference/android/content/Context.html) could be memory mapped. Since this is a legitimate use of the flag, we could just suppress the warning using @SuppressWarnings("deprecation"). https://codereview.chromium.org/684163002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:512: Log.i(TAG, "Fallback process took " + (after - before) + "ms"); On 2014/10/29 13:35:35, Anton wrote: > "Building fallback library took .." Done.
https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:205: if (!Linker.checkLibraryAlignedInApk(apkFilename, libFilename)) { I think that we should really be checking both alignment and that it is uncompressed. Currently only alignment is checked. This means changes in stuff which has already landed. So probably best to do as a separate change.
I think a lot of the fallback code can be deleted if you reuse unpackLibrariesOnce (with some possible adaptions). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:201: String zipFilename = null; zipFilePath and libFilePath https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:203: if (apkFilename != null && Linker.isInZipFile()) { Do we want to add sLibraryLoadFromApkSupported here? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:294: private static void loadLibrary(@Nullable String zipFilename, String libFilename) { zipFilePath and libFilePath https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:53: private static final String FALLBACK_DIR = "fallback"; Could we try integrating this with the other fallback mechanism (pulling it out into the same directory, etc.)? I would like to avoid having two completely independent fallback mechanisms then we will have no clear idea where the library file ended up depending on the fallback which was taken. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:478: ZipFile zip = openZipFile(zipFilename); Why can't we just use unpackLibrariesOnce (possibly adapted somewhat) rather than adding yet more code to pull the libraries out of the zip file? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:512: Log.i(TAG, "Building fallback library took " + (after - before) + "ms"); There is no point logging this I don't think (other than sanity checking during your development). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (left): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:729: assert !isChromiumLinkerLibrary(library); Do you need to remove this assert or is it ok to assert this for both InZip and non-InZip? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:712: public static void loadLibrary(@Nullable String zipFilename, String libFilename) { zipFilePath and libFilePath https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:814: public static String getLibraryFilenameInZipFile(String library) { getLibraryFilePathInZipFile (and same for native) https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... base/android/linker/linker_jni.cc:582: const size_t kMaxFilenameInZip = 256; Could we just export this in crazy_linker.h instead of duplicating it? https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp (right): https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:23: // Maximum filename length of a file in a zip file. filepath https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:402: String LibraryList::GetLibraryFilenameInZipFile(const char* lib_name) { GetLibraryFilePathInZipFile
picksi@chromium.org changed reviewers: + picksi@chromium.org
More of my usual nit-picky naming issues! https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:386: name.startsWith(TEMP_PREFIX + LIBRARY_PREFIX))) { If this were me I'd probably write a function that takes the File and figures out if it is a fallback file (as it is long and not necessarily clear why it is doing the checks it is), e.g. if (isAFallbackFile(f)) {...} It also keeps knowledge of the name out of this function. But this would bloat the code & I know brevity is desirable too! https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:476: should 'before' and 'after' be called startTime & endTime? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:480: should "tmpLib" be called tempLibraryFile? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:483: ZipEntry entry = findZipEntry(zip, library); The name 'entry' doesn't help understand what this is. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:492: // Modify permissions of the temporary file and rename it to fallback library. Is the above comment correct? https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:733: if ((sInBrowserProcess && sBrowserUsesSharedRelro) || sWaitForSharedRelros) { should loadAddress be unintialised and then set to 0 in the else bit of the if? This helps with breakpoints and pointless double initialisation. https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... base/android/linker/linker_jni.cc:598: lib_name_c_str, buffer, sizeof(buffer))) { Are the backwards <constant>!=<variable> part of the coding standards? They're less intuitive to read. Also != SUCCESS is confusing. Is this a binary enum, i.e. can we do == FAILURE instead? https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:380: if (file_name.size() >= buffer_size) { For discussion/thought: should the failure mode here also set buffer[0] to 0 instead of leaving it uninitialised? https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:384: strncpy(buffer, file_name.c_str(), file_name.size() + 1); generally strncpy() would get the sizeof(buffer) passed as the last parameter. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp (right): https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:28: Wasn't there another (existing) constant for kPageSize or is that in a different context? https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:418: if (error) { Should '+1 >' be replaced with '>=' instead? I have a general unease throughout this file with the +1s and size()s! should error be called error_reporting (or something) as 'if (error)' is a bit misleading.
On 2014/10/30 10:48:08, picksi1 wrote: > More of my usual nit-picky naming issues! > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > File > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java > (right): > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:386: > name.startsWith(TEMP_PREFIX + LIBRARY_PREFIX))) { > If this were me I'd probably write a function that takes the File and figures > out if it is a fallback file (as it is long and not necessarily clear why it is > doing the checks it is), e.g. > > if (isAFallbackFile(f)) {...} > > It also keeps knowledge of the name out of this function. > > But this would bloat the code & I know brevity is desirable too! > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:476: > > should 'before' and 'after' be called startTime & endTime? > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:480: > > should "tmpLib" be called tempLibraryFile? > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:483: > ZipEntry entry = findZipEntry(zip, library); > The name 'entry' doesn't help understand what this is. > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:492: > // Modify permissions of the temporary file and rename it to fallback library. > Is the above comment correct? > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): > > https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/Linker.java:733: if > ((sInBrowserProcess && sBrowserUsesSharedRelro) || sWaitForSharedRelros) { > should loadAddress be unintialised and then set to 0 in the else bit of the if? > This helps with breakpoints and pointless double initialisation. > > https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... > File base/android/linker/linker_jni.cc (right): > > https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... > base/android/linker/linker_jni.cc:598: lib_name_c_str, buffer, sizeof(buffer))) > { > Are the backwards <constant>!=<variable> part of the coding standards? They're > less intuitive to read. > > Also != SUCCESS is confusing. Is this a binary enum, i.e. can we do == FAILURE > instead? > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:380: if > (file_name.size() >= buffer_size) { > For discussion/thought: should the failure mode here also set buffer[0] to 0 > instead of leaving it uninitialised? > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:384: > strncpy(buffer, file_name.c_str(), file_name.size() + 1); > generally strncpy() would get the sizeof(buffer) passed as the last parameter. > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp > (right): > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:28: > Wasn't there another (existing) constant for kPageSize or is that in a different > context? > > https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... > third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:418: if > (error) { > Should '+1 >' be replaced with '>=' instead? I have a general unease throughout > this file with the +1s and size()s! > > should error be called error_reporting (or something) as 'if (error)' is a bit > misleading. Also meant to add - could you add the tracking bug (BUG=390618) to the description please.
Thanks for your comments. PTAL. Petr https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:201: String zipFilename = null; On 2014/10/29 17:20:43, rmcilroy wrote: > zipFilePath and libFilePath Done (changed apkFilename to apkFilePath as well). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:203: if (apkFilename != null && Linker.isInZipFile()) { On 2014/10/29 17:20:43, rmcilroy wrote: > Do we want to add sLibraryLoadFromApkSupported here? Probably not because Anton's fallback for devices with the SELinux permission problem is in crazy_linker_elf_loader.cpp (https://codereview.chromium.org/692593002/). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:205: if (!Linker.checkLibraryAlignedInApk(apkFilename, libFilename)) { On 2014/10/29 14:18:26, Anton wrote: > I think that we should really be checking both alignment and that it is > uncompressed. Currently only alignment is checked. > > This means changes in stuff which has already landed. So probably best to do as > a separate change. Agreed (will do in a separate patch). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:294: private static void loadLibrary(@Nullable String zipFilename, String libFilename) { On 2014/10/29 17:20:43, rmcilroy wrote: > zipFilePath and libFilePath Done. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:53: private static final String FALLBACK_DIR = "fallback"; On 2014/10/29 17:20:43, rmcilroy wrote: > Could we try integrating this with the other fallback mechanism (pulling it out > into the same directory, etc.)? I would like to avoid having two completely > independent fallback mechanisms then we will have no clear idea where the > library file ended up depending on the fallback which was taken. I refactored the code and removed duplication where possible. As discussed offline, it is better to keep the (existing) workaround and (new) fallback in separate directories. Fortunately, it seems that we don't actually need Context.MODE_WORLD_READABLE and can use Context.MODE_PRIVATE (as in the workaround). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:386: name.startsWith(TEMP_PREFIX + LIBRARY_PREFIX))) { On 2014/10/30 10:48:07, picksi1 wrote: > If this were me I'd probably write a function that takes the File and figures > out if it is a fallback file (as it is long and not necessarily clear why it is > doing the checks it is), e.g. > > if (isAFallbackFile(f)) {...} > > It also keeps knowledge of the name out of this function. > > But this would bloat the code & I know brevity is desirable too! I removed this code. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:476: On 2014/10/30 10:48:07, picksi1 wrote: > should 'before' and 'after' be called startTime & endTime? ditto. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:478: ZipFile zip = openZipFile(zipFilename); On 2014/10/29 17:20:44, rmcilroy wrote: > Why can't we just use unpackLibrariesOnce (possibly adapted somewhat) rather > than adding yet more code to pull the libraries out of the zip file? I refactored the code and merged them where I could (see unpackLibraries). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:480: On 2014/10/30 10:48:07, picksi1 wrote: > should "tmpLib" be called tempLibraryFile? ditto (removed code). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:483: ZipEntry entry = findZipEntry(zip, library); On 2014/10/30 10:48:07, picksi1 wrote: > The name 'entry' doesn't help understand what this is. I renamed it to "packedLib". https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:492: // Modify permissions of the temporary file and rename it to fallback library. On 2014/10/30 10:48:07, picksi1 wrote: > Is the above comment correct? ditto (but I think that it was). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:512: Log.i(TAG, "Building fallback library took " + (after - before) + "ms"); On 2014/10/29 17:20:43, rmcilroy wrote: > There is no point logging this I don't think (other than sanity checking during > your development). Done. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (left): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:729: assert !isChromiumLinkerLibrary(library); On 2014/10/29 17:20:44, rmcilroy wrote: > Do you need to remove this assert or is it ok to assert this for both InZip and > non-InZip? The problem here is that libFilePath is "libchromium_android_linker.so" rather than "chromium_android_linker". This needs to be the case because the fallback creates a special library file which is passed here as the parameter. If you really want to have some sort of a check here, we would probably have to add a new method (e.g. isChromiumLinkerLibraryPath). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:712: public static void loadLibrary(@Nullable String zipFilename, String libFilename) { On 2014/10/29 17:20:44, rmcilroy wrote: > zipFilePath and libFilePath Done. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:733: if ((sInBrowserProcess && sBrowserUsesSharedRelro) || sWaitForSharedRelros) { On 2014/10/30 10:48:07, picksi1 wrote: > should loadAddress be unintialised and then set to 0 in the else bit of the if? > This helps with breakpoints and pointless double initialisation. You're probably right but I don't want to touch this code in this patch (except for the variable names). https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:814: public static String getLibraryFilenameInZipFile(String library) { On 2014/10/29 17:20:44, rmcilroy wrote: > getLibraryFilePathInZipFile (and same for native) Done (all the way down to crazy_linker_library_list.cpp). https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... base/android/linker/linker_jni.cc:582: const size_t kMaxFilenameInZip = 256; On 2014/10/29 17:20:44, rmcilroy wrote: > Could we just export this in crazy_linker.h instead of duplicating it? Done. I moved the constant to crazy_linker.h so crazy_linker_library_list.cpp now includes "<crazy_linker.h>". If we wanted to avoid this, we would probably need to add a new header file (e.g. "crazy_linker_constants.h"). https://codereview.chromium.org/684163002/diff/40001/base/android/linker/link... base/android/linker/linker_jni.cc:598: lib_name_c_str, buffer, sizeof(buffer))) { On 2014/10/30 10:48:08, picksi1 wrote: > Are the backwards <constant>!=<variable> part of the coding standards? They're > less intuitive to read. > > Also != SUCCESS is confusing. Is this a binary enum, i.e. can we do == FAILURE > instead? Done. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:380: if (file_name.size() >= buffer_size) { On 2014/10/30 10:48:08, picksi1 wrote: > For discussion/thought: should the failure mode here also set buffer[0] to 0 > instead of leaving it uninitialised? I would personally keep it as is - the function doesn't specify what it does with the buffer in case of a failure. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:384: strncpy(buffer, file_name.c_str(), file_name.size() + 1); On 2014/10/30 10:48:08, picksi1 wrote: > generally strncpy() would get the sizeof(buffer) passed as the last parameter. I use "file_name.size() + 1" here because I know that "file_name.size() < buffer_size" and I get a '\0' at the end for free. If you find this difficult to understand or error-prone, I will change it. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp (right): https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:23: // Maximum filename length of a file in a zip file. On 2014/10/29 17:20:44, rmcilroy wrote: > filepath Done (changed to "path"). Also changed the name of the constant to kMaxFilePathLengthInZip. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:28: On 2014/10/30 10:48:08, picksi1 wrote: > Wasn't there another (existing) constant for kPageSize or is that in a different > context? This is the one we discussed in my previous patch (https://codereview.chromium.org/684453003/). The previous patch wasn't rebased yet (the new one is). https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:402: String LibraryList::GetLibraryFilenameInZipFile(const char* lib_name) { On 2014/10/29 17:20:44, rmcilroy wrote: > GetLibraryFilePathInZipFile Done. https://codereview.chromium.org/684163002/diff/40001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:418: if (error) { On 2014/10/30 10:48:08, picksi1 wrote: > Should '+1 >' be replaced with '>=' instead? I have a general unease throughout > this file with the +1s and size()s! > > should error be called error_reporting (or something) as 'if (error)' is a bit > misleading. Again, this if statement was removed in my previous CL (https://codereview.chromium.org/684453003/). I also changed it to ">=" as you suggested.
https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:144: return context.getDir(dirName, Context.MODE_PRIVATE); Is MODE_PRIVATE giving you a directory that both the renderer and browser can read from on K and L? https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:162: private static boolean unpackWorkaroundLibrariesOnce(Context context) { As you made changes here you probably should figure out how to check the original purpose of this code (on the Xperia, if I remember correctly).
Generally looks good. A couple more comments. https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:203: if (apkFilename != null && Linker.isInZipFile()) { On 2014/10/30 19:55:06, petrcermak wrote: > On 2014/10/29 17:20:43, rmcilroy wrote: > > Do we want to add sLibraryLoadFromApkSupported here? > > Probably not because Anton's fallback for devices with the SELinux permission > problem is in crazy_linker_elf_loader.cpp > (https://codereview.chromium.org/692593002/). Acknowledged. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:185: Log.w(TAG, "loadAlreadyLocked(): null context"); nit - could not check load from APK support due to null context" https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:56: // Workaround and fallback directories. nit - add a TODO that we should use the same directory for both once the refactor has been done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:57: public static final String WORKAROUND_DIR = "lib"; nit - PACKAGE_MANAGER_WORKAROUND_DIR https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:58: public static final String FALLBACK_DIR = "fallback"; LOAD_FROM_APK_FALLBACK_DIR https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:162: private static boolean unpackWorkaroundLibrariesOnce(Context context) { On 2014/10/31 11:28:56, Anton wrote: > As you made changes here you probably should figure out how to check the > original purpose of this code (on the Xperia, if I remember correctly). It was also a specific version of Android (JellyBean or ICS if I remember correctly). https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:217: * directly from the zip file. nit - /zip file/APK. Also please link to the BUG https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:244: private static void deleteObsoleteLibraries(File libDir, Do we only call this when performing the fallback? We should probably delete the files if they exist due to a previous fallback if we are no longer requiring the fallback. Would this be possible? https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:245: @Nullable Collection<File> keptFiles) { Just make this non-nullable and have an empty collection if there are no keptFiles - this would simplify the checking code. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:258: if (files != null) { listFiles doesn't return null so no need for the null check https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:354: InputStream in = null; do you need the "= null" here? https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:372: OutputStream out = null; ditto https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:377: throw new UnpackingException("failed to create temporary file", e); it's not a temporary file - replace with "unpacked library file" throughout
Generally looks good. A couple more comments.
https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:384: strncpy(buffer, path.c_str(), path.size() + 1); Better to use memcpy here, since you know categorically the length of the string (saves a test for nul on each char copied). https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp (right): https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:418: error->Format("Filename too long for a file in a zip file %s\n", Tab character here. And on the next line.
Also, please add tracking bug number for direct load from apk to the description (BUG=390618).
Thanks for your comments. PTAL. Petr https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:185: Log.w(TAG, "loadAlreadyLocked(): null context"); On 2014/10/31 14:20:21, rmcilroy wrote: > nit - could not check load from APK support due to null context" Done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:56: // Workaround and fallback directories. On 2014/10/31 14:20:21, rmcilroy wrote: > nit - add a TODO that we should use the same directory for both once the > refactor has been done. Done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:57: public static final String WORKAROUND_DIR = "lib"; On 2014/10/31 14:20:21, rmcilroy wrote: > nit - PACKAGE_MANAGER_WORKAROUND_DIR Done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:58: public static final String FALLBACK_DIR = "fallback"; On 2014/10/31 14:20:21, rmcilroy wrote: > LOAD_FROM_APK_FALLBACK_DIR Done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:144: return context.getDir(dirName, Context.MODE_PRIVATE); On 2014/10/31 11:28:56, Anton wrote: > Is MODE_PRIVATE giving you a directory that both the renderer and browser can > read from on K and L? Surprisingly, yes. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:162: private static boolean unpackWorkaroundLibrariesOnce(Context context) { On 2014/10/31 14:20:21, rmcilroy wrote: > On 2014/10/31 11:28:56, Anton wrote: > > As you made changes here you probably should figure out how to check the > > original purpose of this code (on the Xperia, if I remember correctly). > > It was also a specific version of Android (JellyBean or ICS if I remember > correctly). I tried to reproduce the bug on Sony Xperia (but didn't manage to). Everything seems to work fine. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:217: * directly from the zip file. On 2014/10/31 14:20:21, rmcilroy wrote: > nit - /zip file/APK. Also please link to the BUG Done (linked to crbug.com/390618). https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:244: private static void deleteObsoleteLibraries(File libDir, On 2014/10/31 14:20:21, rmcilroy wrote: > Do we only call this when performing the fallback? We should probably delete > the files if they exist due to a previous fallback if we are no longer requiring > the fallback. Would this be possible? Done. I added it to LibraryLoader.loadAlreadyLocked(). The fallback folder will be deleted if it was not used by any library. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:245: @Nullable Collection<File> keptFiles) { On 2014/10/31 14:20:21, rmcilroy wrote: > Just make this non-nullable and have an empty collection if there are no > keptFiles - this would simplify the checking code. Done. https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:258: if (files != null) { On 2014/10/31 14:20:21, rmcilroy wrote: > listFiles doesn't return null so no need for the null check It does: http://docs.oracle.com/javase/7/docs/api/java/io/File.html#listFiles() https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:354: InputStream in = null; On 2014/10/31 14:20:21, rmcilroy wrote: > do you need the "= null" here? Done (no, we don't). https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:372: OutputStream out = null; On 2014/10/31 14:20:21, rmcilroy wrote: > ditto ditto :-) https://codereview.chromium.org/684163002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:377: throw new UnpackingException("failed to create temporary file", e); On 2014/10/31 14:20:21, rmcilroy wrote: > it's not a temporary file - replace with "unpacked library file" throughout Done. https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:384: strncpy(buffer, path.c_str(), path.size() + 1); On 2014/10/31 17:48:45, simonb wrote: > Better to use memcpy here, since you know categorically the length of the string > (saves a test for nul on each char copied). Done. https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp (right): https://codereview.chromium.org/684163002/diff/60001/third_party/android_craz... third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:418: error->Format("Filename too long for a file in a zip file %s\n", On 2014/10/31 17:48:45, simonb wrote: > Tab character here. And on the next line. Done.
lgtm once comments are addressed, thanks! https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:393: throw new UnpackingException("failed to open output stream", e); failed to open output stream for unpacked library file. https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:437: ((ZipFile) closeable).close(); I'm not keen on this hack - I think it would be better just to have two separate methods with different argument types.
I am impressed by your dedication to get into M40! Simon On 2 Nov 2014 14:12, <rmcilroy@chromium.org> wrote: > lgtm once comments are addressed, thanks! > > > https://codereview.chromium.org/684163002/diff/80001/base/ > android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java > File > base/android/java/src/org/chromium/base/library_loader/ > LibraryLoaderHelper.java > (right): > > https://codereview.chromium.org/684163002/diff/80001/base/ > android/java/src/org/chromium/base/library_loader/ > LibraryLoaderHelper.java#newcode393 > base/android/java/src/org/chromium/base/library_loader/ > LibraryLoaderHelper.java:393: > throw new UnpackingException("failed to open output stream", e); > failed to open output stream for unpacked library file. > > https://codereview.chromium.org/684163002/diff/80001/base/ > android/java/src/org/chromium/base/library_loader/ > LibraryLoaderHelper.java#newcode437 > base/android/java/src/org/chromium/base/library_loader/ > LibraryLoaderHelper.java:437: > ((ZipFile) closeable).close(); > I'm not keen on this hack - I think it would be better just to have two > separate methods with different argument types. > > https://codereview.chromium.org/684163002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for your comments. I will commit this patch tomorrow. Petr https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java (right): https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:393: throw new UnpackingException("failed to open output stream", e); On 2014/11/02 14:12:09, rmcilroy wrote: > failed to open output stream for unpacked library file. Done. https://codereview.chromium.org/684163002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:437: ((ZipFile) closeable).close(); On 2014/11/02 14:12:09, rmcilroy wrote: > I'm not keen on this hack - I think it would be better just to have two separate > methods with different argument types. Done (added a closeZipFile method).
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/684163002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/51ca0822d0f57cd8f6dd82dd6fe49ab6c14c1b1b Cr-Commit-Position: refs/heads/master@{#302456} |
