Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1665513002:
Pass both 32 and 64 bit snapshot and natives fds to child processes. (Closed)
|
Created:
4 years, 10 months ago by Tobias Sargeant Modified:
4 years, 10 months ago CC:
android-webview-reviews_chromium.org, avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass both 32 and 64 bit snapshot and natives fds to child processes.
Child processes are in the best position to determine which files
to use, therefore it is simplest just to provide both 32 and 64
bit versions from the parent.
BUG=581409, 455699
Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1
Cr-Commit-Position: refs/heads/master@{#374371}
Committed: https://crrev.com/b2001627a9e1dcf3df5218424a55e32143b957b1
Cr-Commit-Position: refs/heads/master@{#374643}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 2
Patch Set 3 : fix compile error on non android platforms #Patch Set 4 : avoid non-POD global #
Messages
Total messages: 38 (16 generated)
tobiasjs@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/1665513002/diff/1/android_webview/lib/DEPS File android_webview/lib/DEPS (right): https://codereview.chromium.org/1665513002/diff/1/android_webview/lib/DEPS#ne... android_webview/lib/DEPS:6: "+gin/v8_initializer.h", This is probably not okay, only headers in public dirs are supposed to be used by other components. https://codereview.chromium.org/1665513002/diff/1/content/browser/child_proce... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/1665513002/diff/1/content/browser/child_proce... content/browser/child_process_launcher.cc:182: snapshot_loaded = true; Setting this to true unconditionally seems maybe wrong? https://codereview.chromium.org/1665513002/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1665513002/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.cc:1231: new_frame->metadata()->SetBoolean( Did you mean to change this?
https://codereview.chromium.org/1665513002/diff/1/android_webview/lib/DEPS File android_webview/lib/DEPS (right): https://codereview.chromium.org/1665513002/diff/1/android_webview/lib/DEPS#ne... android_webview/lib/DEPS:6: "+gin/v8_initializer.h", On 2016/02/03 11:05:32, Torne wrote: > This is probably not okay, only headers in public dirs are supposed to be used > by other components. I agree, that it looks wrong, but there are plenty of other places that include this header specifically, and apply a specific DEPS rule to do so. It seems like this should actually be public. https://codereview.chromium.org/1665513002/diff/1/content/browser/child_proce... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/1665513002/diff/1/content/browser/child_proce... content/browser/child_process_launcher.cc:182: snapshot_loaded = true; On 2016/02/03 11:05:32, Torne wrote: > Setting this to true unconditionally seems maybe wrong? It's irritating, but I'm not sure what set of FDs we can reasonably test, because we could be: * a 32 bit monochrome with no 64 bit snapshots [64 bit fails] * a 64 bit only monochrome [32 bit fails] * a 64/32 bit monochrome, with either a 32 or 64 bit broser process [both load] I can't see how we can't tell which bitness we need to succeed in order to start a child (and in fact if we knew that, we wouldn't need to pass both sets). For that reason, I think the only thing to do is to assume that everything is OK, and let the child process complain if it isn't. https://codereview.chromium.org/1665513002/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1665513002/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.cc:1231: new_frame->metadata()->SetBoolean( On 2016/02/03 11:05:32, Torne wrote: > Did you mean to change this? No, comes from another CL. Will remove.
tobiasjs@chromium.org changed reviewers: + jochen@chromium.org
can you explain why we don't statically know which files we need? Either the process is 64 bit or not, right?
On 2016/02/03 13:36:12, jochen wrote: > can you explain why we don't statically know which files we need? Either the > process is 64 bit or not, right? Webview is considering a multiprocess mode where the choice of renderer process ABI is made by factors out of our control, and can be different (in either direction) to the ABI of the broswer process. See: crbug.com/581380 and crbug.com/581409.
lgtm if torne is happy with this
LGTM, just a comment about one of the todos being deleted :) https://codereview.chromium.org/1665513002/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (left): https://codereview.chromium.org/1665513002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:133: // here when those files have arch specific names http://crbug.com/455699 Does this CL implicitly clear up the issue from that crbug? We should probably update it?
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380 ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 ==========
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665513002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665513002/20001
https://codereview.chromium.org/1665513002/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (left): https://codereview.chromium.org/1665513002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:133: // here when those files have arch specific names http://crbug.com/455699 On 2016/02/08 15:57:34, Torne wrote: > Does this CL implicitly clear up the issue from that crbug? We should probably > update it? The bug is probably not completely resolved, but given that the copying behaviour has now propagated to gn, it seems unlikely that anyone is going to go ahead and improve the snapshot/natives build steps. I'm closing the bug because it seems like this TODO is the last other reference to it.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/1665513002/#ps40001 (title: "fix compile error on non android platforms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665513002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665513002/40001
Message was sent while issue was closed.
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1681003003/ by thakis@chromium.org. The reason for reverting is: Added a static initializer: https://build.chromium.org/p/chromium/builders/Linux/builds/71576 # v8_initializer.cc _GLOBAL__sub_I_v8_initializer.cc+0xf # v8_initializer.cc __cxa_atexit@plt [registers a dtor to run at exit].
Message was sent while issue was closed.
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581380,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581409,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ==========
The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665513002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665513002/60001
Hi Torne, Could you please take a look at PS4?
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1665513002/#ps60001 (title: "avoid non-POD global")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665513002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665513002/60001
Message was sent while issue was closed.
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581409,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581409,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581409,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} ========== to ========== Pass both 32 and 64 bit snapshot and natives fds to child processes. Child processes are in the best position to determine which files to use, therefore it is simplest just to provide both 32 and 64 bit versions from the parent. BUG=581409,455699 Committed: https://crrev.com/c560d75783aca05249092dd11503b53f7b631be1 Cr-Commit-Position: refs/heads/master@{#374371} Committed: https://crrev.com/b2001627a9e1dcf3df5218424a55e32143b957b1 Cr-Commit-Position: refs/heads/master@{#374643} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b2001627a9e1dcf3df5218424a55e32143b957b1 Cr-Commit-Position: refs/heads/master@{#374643} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
