Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406083002:
Crazylinker: remove the code to load component build library (Closed)
|
Created:
4 years, 2 months ago by michaelbai Modified:
4 years, 1 month ago Reviewers:
Torne CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrazylinker: remove the code to load component build library
The code is broken, component build library will not be loaded if
non-component build library is in system lib path, e.g. Chrome is
preinstalled.
BUG=657093
Committed: https://crrev.com/275c325caaec3cc65f0c3be9af0abd00963e9d01
Cr-Commit-Position: refs/heads/master@{#427809}
Patch Set 1 #
Total comments: 1
Patch Set 2 : remove the code #
Total comments: 2
Patch Set 3 : remove one more component code #
Total comments: 2
Patch Set 4 : change comment #
Messages
Total messages: 23 (8 generated)
Description was changed from ========== Add comment for crazylinker for potential component failure. BUG= ========== to ========== Add comment for crazylinker for potential component failure. BUG= ==========
michaelbai@chromium.org changed reviewers: + torne@chromium.org
Description was changed from ========== Add comment for crazylinker for potential component failure. BUG= ========== to ========== Add comment in crazylinker for potential component failure. BUG= ==========
I don't understand what you mean here; can you try to explain differently?
https://codereview.chromium.org/2406083002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/2406083002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:534: System.loadLibrary(LINKER_JNI_LIBRARY + ".cr"); If you don't think crazy linker will support component build, I could just remove this.
I don't get what the issue is here at all, so I'm not sure what is the right thing to do.
On 2016/10/12 12:10:55, Torne wrote: > I don't get what the issue is here at all, so I'm not sure what is the right > thing to do. Oh, the problem is that the code wants to load the flat crazylinker shared library first and if it fails, then try component build, but it will not work in most device that has chrome previous, because libchromium_android_linker.so could already be in system lib path, if Chrome is component build, it actually loads the wrong crazylinker's so file which is not the one in APK.
Description was changed from ========== Add comment in crazylinker for potential component failure. BUG= ========== to ========== Crazylinker: remove the code to load component build library The code is broken, component build library will not be loaded if non-component build library is in system lib path, e.g. Chrome is preinstalled. BUG=657093 ==========
https://codereview.chromium.org/2406083002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/2406083002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:513: return library.equals(LINKER_JNI_LIBRARY) || library.equals(LINKER_JNI_LIBRARY + ".cr"); Seems like we should also remove the .cr case here?
PTAL https://codereview.chromium.org/2406083002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/2406083002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:513: return library.equals(LINKER_JNI_LIBRARY) || library.equals(LINKER_JNI_LIBRARY + ".cr"); On 2016/10/24 12:07:03, Torne wrote: > Seems like we should also remove the .cr case here? Done.
LGTM with a comment nit https://codereview.chromium.org/2406083002/diff/40001/base/android/linker/con... File base/android/linker/config.gni (right): https://codereview.chromium.org/2406083002/diff/40001/base/android/linker/con... base/android/linker/config.gni:12: # and needs to cleanup. See crbug.com/657093. nit: this comment is hard to understand; it sounds like someone is intending to fix this at some point, whereas it's actually the opposite. Keeping the comment about it being broken on android 4.4 also makes it less clear. I'd suggest just replacing both comments with something like: # Chromium linker doesn't reliably support loading multiple libraries; disable for component builds. What do you think?
https://codereview.chromium.org/2406083002/diff/40001/base/android/linker/con... File base/android/linker/config.gni (right): https://codereview.chromium.org/2406083002/diff/40001/base/android/linker/con... base/android/linker/config.gni:12: # and needs to cleanup. See crbug.com/657093. On 2016/10/26 10:27:44, Torne wrote: > nit: this comment is hard to understand; it sounds like someone is intending to > fix this at some point, whereas it's actually the opposite. Keeping the comment > about it being broken on android 4.4 also makes it less clear. > > I'd suggest just replacing both comments with something like: > > # Chromium linker doesn't reliably support loading multiple libraries; disable > for component builds. > > What do you think? Done.
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2406083002/#ps60001 (title: "change comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Crazylinker: remove the code to load component build library The code is broken, component build library will not be loaded if non-component build library is in system lib path, e.g. Chrome is preinstalled. BUG=657093 ========== to ========== Crazylinker: remove the code to load component build library The code is broken, component build library will not be loaded if non-component build library is in system lib path, e.g. Chrome is preinstalled. BUG=657093 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Crazylinker: remove the code to load component build library The code is broken, component build library will not be loaded if non-component build library is in system lib path, e.g. Chrome is preinstalled. BUG=657093 ========== to ========== Crazylinker: remove the code to load component build library The code is broken, component build library will not be loaded if non-component build library is in system lib path, e.g. Chrome is preinstalled. BUG=657093 Committed: https://crrev.com/275c325caaec3cc65f0c3be9af0abd00963e9d01 Cr-Commit-Position: refs/heads/master@{#427809} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/275c325caaec3cc65f0c3be9af0abd00963e9d01 Cr-Commit-Position: refs/heads/master@{#427809}
Message was sent while issue was closed.
On 2016/10/26 21:33:56, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/275c325caaec3cc65f0c3be9af0abd00963e9d01 > Cr-Commit-Position: refs/heads/master@{#427809} I meant replace the comments about component builds (the one existing one and the one you were adding), not the comment about instrumentation, which seems still relevant as it explains why all the other conditions are there.
Message was sent while issue was closed.
On 2016/10/27 15:17:28, Torne wrote: > On 2016/10/26 21:33:56, commit-bot: I haz the power wrote: > > Patchset 4 (id:??) landed as > > https://crrev.com/275c325caaec3cc65f0c3be9af0abd00963e9d01 > > Cr-Commit-Position: refs/heads/master@{#427809} > > I meant replace the comments about component builds (the one existing one and > the one you were adding), not the comment about instrumentation, which seems > still relevant as it explains why all the other conditions are there. OK, revert it
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2453683007/ by michaelbai@chromium.org. The reason for reverting is: Wrong change about comment. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
