Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1101473006:
Prep for passing fallback descriptor between browser and child processes. (Closed)
DescriptionPrep for passing fallback descriptor between browser and child processes.
A subsequent change will pass the fallback descriptor between the linker in
the parent process and the linker in the child process.
* Change the linker to always wait for Bundle from parent in child.
* Rename linker methods for getting and passing bundle:
s/Linker.getSharedRelros/Linker.produceSharedStateBundle/
s/Linker.useSharedRelros/Linker.consumeSharedStateBundle/
* ensure Linker.initService called before Linker used in ChildProcessService.
BUG=458056
Patch Set 1 #
Total comments: 25
Patch Set 2 : Incorporate comments from Patch Set 1 review. #
Total comments: 4
Patch Set 3 : Incorporate comments from petrcermak on Patch Set 2. #Patch Set 4 : Fix missing TODO. #Patch Set 5 : Rebase from master. #
Messages
Total messages: 26 (10 generated)
oth@chromium.org changed reviewers: + jochen@chromium.org, petrcermak@chromium.org, rmcilroy@chromium.org
rmcilroy@chromium.org, petrcermak@chromium: Please review the the changes here - it's part 1/2 for this issue. This change incorporates petrcermak's changes in https://codereview.chromium.org/930333003/ jochen@chromium.org: Please review changes in content/public/android/java/src/org/chromium/content/... Thanks!
i'm not a good reviewer for java changes, sorry
The change looks very good overall. Just a couple of style comments ... (I'll leave it up to rmcilroy to approve the patch) Thanks, Petr https://codereview.chromium.org/1101473006/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/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:145: * will include fallback library descriptors. I wouldn't say "will include fallback library descriptors". Instead, I'd add a "TODO(oth): Include fallback library descriptors". https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:152: * When the Bundle is received the service process must call nit: comma after "received" https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:419: String.format(Locale.US, "sInBrowserProcess=%s sBrowserUsesSharedRelro=%s", Is there any reason to change the indentation here? https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:478: * (these will have be produced with produceSharedStateBundle). s/be/been/ https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:550: assert sSharedRelros == null : "Should initially be empty for non-browser."; You should probably say *what* "should initially be empty" ("Bundle"?). https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:577: String.format(Locale.US, "enableSharedRelros(0x%x) called", baseLoadAddress)); I wouldn't change the indentation here either. https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/app/ChromiumLinkerParams.java (right): https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/app/ChromiumLinkerParams.java:57: return String.format("LinkerParams(baseLoadAddress:0x%x, testRunnerClassName:%s", You know what I'm going to say :-) https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:266: public void setupConnection(String[] commandLine, FileDescriptorInfo[] filesToBeMapped, I think that the signature is a little easier to read when each argument is on a separate line.
Looks great. +1 to Pete's comments and a few more nits. https://codereview.chromium.org/1101473006/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/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:115: * will behave exactly like System.loadLibrary(). I'm not sure this is true anymore (that the crazy linker behaves 'exactly' like the System linker - maybe just drop everything after the command. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:210: private static volatile Bundle sSharedRelros = null; Does this need to be volatile? It shoukd only accessed within synchronised i think? https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:480: * finishLibraryLoad() will not exit until this method is called in another Nit - fix the line breaks here (so the whole comment uses 100 char lines). https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:528: * <p>Only valid for browser processes and after loading all libraries. Nit - I wouldn't bother with the <p> - I don't think anyone javadocs this. Either way, add the new line to the comment to so it would be consistent with javadoc output.
oth@chromium.org changed reviewers: - jochen@chromium.org
Good feedback, incorporated all. Thanks. https://codereview.chromium.org/1101473006/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/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:115: * will behave exactly like System.loadLibrary(). On 2015/04/23 09:03:27, rmcilroy wrote: > I'm not sure this is true anymore (that the crazy linker behaves 'exactly' like > the System linker - maybe just drop everything after the command. Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:145: * will include fallback library descriptors. On 2015/04/22 15:14:45, petrcermak wrote: > I wouldn't say "will include fallback library descriptors". Instead, I'd add a > "TODO(oth): Include fallback library descriptors". Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:152: * When the Bundle is received the service process must call On 2015/04/22 15:14:44, petrcermak wrote: > nit: comma after "received" Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:210: private static volatile Bundle sSharedRelros = null; On 2015/04/23 09:03:26, rmcilroy wrote: > Does this need to be volatile? It shoukd only accessed within synchronised i > think? Yes. Fixed. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:419: String.format(Locale.US, "sInBrowserProcess=%s sBrowserUsesSharedRelro=%s", On 2015/04/22 15:14:44, petrcermak wrote: > Is there any reason to change the indentation here? I'll change these back - this is a 'git cl format' side-effect. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:478: * (these will have be produced with produceSharedStateBundle). On 2015/04/22 15:14:45, petrcermak wrote: > s/be/been/ Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:480: * finishLibraryLoad() will not exit until this method is called in another On 2015/04/23 09:03:26, rmcilroy wrote: > Nit - fix the line breaks here (so the whole comment uses 100 char lines). Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:528: * <p>Only valid for browser processes and after loading all libraries. On 2015/04/23 09:03:27, rmcilroy wrote: > Nit - I wouldn't bother with the <p> - I don't think anyone javadocs this. > Either way, add the new line to the comment to so it would be consistent with > javadoc output. Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:550: assert sSharedRelros == null : "Should initially be empty for non-browser."; On 2015/04/22 15:14:44, petrcermak wrote: > You should probably say *what* "should initially be empty" ("Bundle"?). Done. https://codereview.chromium.org/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:577: String.format(Locale.US, "enableSharedRelros(0x%x) called", baseLoadAddress)); On 2015/04/22 15:14:44, petrcermak wrote: > I wouldn't change the indentation here either. Done. https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/app/ChromiumLinkerParams.java (right): https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/app/ChromiumLinkerParams.java:57: return String.format("LinkerParams(baseLoadAddress:0x%x, testRunnerClassName:%s", On 2015/04/22 15:14:45, petrcermak wrote: > You know what I'm going to say :-) Done. https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/1101473006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:266: public void setupConnection(String[] commandLine, FileDescriptorInfo[] filesToBeMapped, On 2015/04/22 15:14:45, petrcermak wrote: > I think that the signature is a little easier to read when each argument is on a > separate line. Done.
oth@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: Please review changes in content/public/android/java/src/org/chromium/content/{app,browser}
Two more comments. Petr https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:438: assert !sInBrowserProcess; This assertion is unnecessary now. https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:563: sBaseLoadAddress = 0; You should set sBrowserUsesSharedRelro = false here. Otherwise, there's no way to disable shared RELROs in the browser process, which might currently happen (see LibraryLoader.java:378).
Thanks! https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:438: assert !sInBrowserProcess; On 2015/04/23 13:14:53, petrcermak wrote: > This assertion is unnecessary now. Done. https://codereview.chromium.org/1101473006/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:563: sBaseLoadAddress = 0; On 2015/04/23 13:14:53, petrcermak wrote: > You should set sBrowserUsesSharedRelro = false here. Otherwise, there's no way > to disable shared RELROs in the browser process, which might currently happen > (see LibraryLoader.java:378). Good catch. This came from the starting patch and I missed it http://crrev.com/930333003.
LGTM. Thanks :-) Petr
lgtm, thanks! https://codereview.chromium.org/1101473006/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/1101473006/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/library_loader/Linker.java:145: * will include fallback library descriptors. On 2015/04/23 11:20:43, orion wrote: > On 2015/04/22 15:14:45, petrcermak wrote: > > I wouldn't say "will include fallback library descriptors". Instead, I'd add a > > "TODO(oth): Include fallback library descriptors". > > Done. Missed adding the TODO?
rmcilroy@chromium.org changed reviewers: + yfriedman@chromium.org - sievers@chromium.org
-sievers, +yfriedman Yaron, provide a content/public/android review please?
content/public/android lgtm
The CQ bit was checked by oth@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1101473006/#ps60001 (title: "Fix missing TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1101473006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by oth@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, rmcilroy@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1101473006/#ps80001 (title: "Rebase from master.")
The CQ bit was unchecked by oth@chromium.org
On 2015/04/25 00:08:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Should this be closed? Thanks, Petr
Message was sent while issue was closed.
On 2015/07/14 08:57:13, petrcermak wrote: > On 2015/04/25 00:08:40, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > Should this be closed? > > Thanks, > Petr Yes, thanks for flagging. Closed this and will close the follow-on. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
