New issue
Advanced search Search tips

Issue 660325 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Out-of-process component patching is not supported on Android

Project Member Reported by bauerb@chromium.org, Oct 28 2016

Issue description

I got this DCHECK failure on a local Android build today:

[FATAL:child_process_launcher.cc(455)] Check failed: process_type == switches::kGpuProcess || !cmd_line->HasSwitch(switches::kNoSandbox). 

Stack Trace:
  RELADDR   FUNCTION                                           FILE:LINE
  00041f50  tgkill+12                                          /system/lib/libc.so
  0003fb5d  pthread_kill+32                                    /system/lib/libc.so
  0001c30f  raise+10                                           /system/lib/libc.so
  000194c1  __libc_android_abort+34                            /system/lib/libc.so
  000174ac  abort+4                                            /system/lib/libc.so
  v------>  base::debug::(anonymous namespace)::DebugBreak()   /usr/local/google_ssd/build/clankium/src/base/debug/debugger_posix.cc:221
  00081143  base::debug::BreakDebugger()                       /usr/local/google_ssd/build/clankium/src/base/debug/debugger_posix.cc:251
  0009509d  ~LogMessage                                        /usr/local/google_ssd/build/clankium/src/base/logging.cc:748
  006ccfcb  content::ChildProcessLauncher::Launch()            /usr/local/google_ssd/build/clankium/src/content/browser/child_process_launcher.cc:454
  006cce1b  ChildProcessLauncher                               /usr/local/google_ssd/build/clankium/src/content/browser/child_process_launcher.cc:422
  006a099f  content::BrowserChildProcessHostImpl::Launch()     /usr/local/google_ssd/build/clankium/src/content/browser/browser_child_process_host_impl.cc:265
  00908831  content::UtilityProcessHostImpl::StartProcess()    /usr/local/google_ssd/build/clankium/src/content/browser/utility_process_host_impl.cc:344
  009085b1  content::UtilityProcessHostImpl::Send()            /usr/local/google_ssd/build/clankium/src/content/browser/utility_process_host_impl.cc:182
  00398f2b  component_updater::PatchHost::StartProcess()       /usr/local/google_ssd/build/clankium/src/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:65

The comment right above the failing DCHECK (https://cs.chromium.org/chromium/src/content/browser/child_process_launcher.cc?dr=CSs&sq=package:chromium&l=453) says "Non-sandboxed utility or renderer process are currently not supported".
 
Looks like there are two options:

(1) Do the patching in-process on Android.
(2) Figure out how to do the patching in a sandboxed utility process. (Could we replace .DisableSandbox with .SetExposedDir?) Note that we can only expose a single directory this way (it's not AddExposedDir...)
Labels: -Pri-2 Pri-1
Just got bit by this today. Can we resolve quickly or disable the check? This is making running debug builds on Android painful.
Owner: waff...@chromium.org
As an immediate workaround, if you run with a fresh user-data-dir you will avoid any component patching.

I'm not keen on moving the patching in-process on Android due to security reasons, but similarly am unsure about how well switching to a sandboxed utility process will work overall (it would be especially unfortunate if we have to do something different on each of linux, win/mac, ios, and android).

Another option is to disable differential component updates for Android for the time being. (That has the obvious drawback that we'll be wasteful of user's network connections.)

Anyways, I'll take ownership and chat with the security team to figure out the best way forward here.
Thanks, I'll blow away the user dir for now.
Had a chat about this, thinking so far is the browser process should open some FDs and hand them over to a sandboxed utility process to operate with; there are some details in how the files are memory-mapped and how courgette itself is called that need to be figured out but overall I think that's the way we'll go.

Comment 6 by w...@chromium.org, Nov 15 2016

I keep hitting this. Can we disable the DCHECK with a TODO pointing to this bug?
 Issue 668345  has been merged into this issue.
Upvote for #c6

Re #3 should I uninstall and then install Chrome to use a fresh data dir? Should I do it every time I update the build on the device?
Status: Started (was: Untriaged)
Starting work on fixing this now, following the plan described in #5.

Re #7: You shouldn't need to uninstall/reinstall Chrome, it should be sufficient to delete the /data/user/0/com.android.chrome/app_chrome directory. You should only need to do it once per component update; I would guess that once per day is sufficient.
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53796c72e2ec50eec77f0bd79ec0c8426cee4d58

commit 53796c72e2ec50eec77f0bd79ec0c8426cee4d58
Author: waffles <waffles@chromium.org>
Date: Fri Dec 09 22:09:31 2016

Sandbox the component updater's patcher utility process.

The code will now open files in the browser process before passing the
handles across IPC to the utility process. The utility process in turn
invokes courgette/bsdiff, which memory maps the files and operates on
them as before.

There is a behavioral difference when using the courgette or
courgette_mini tools: the output file will now be created/overwritten
at the start of the operation, and in the case of a failure, will be
deleted. Previously, the output file was created late in the operation
operation and several failure modes would leave it unmodified.

BUG= 660325 

Review-Url: https://codereview.chromium.org/2534873005
Cr-Commit-Position: refs/heads/master@{#437669}

[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/chrome/common/chrome_utility_messages.h
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/chrome/utility/chrome_content_utility_client.cc
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/chrome/utility/chrome_content_utility_client.h
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/courgette/courgette.h
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/courgette/ensemble_apply.cc
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/courgette/third_party/bsdiff/bsdiff.h
[modify] https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58/courgette/third_party/bsdiff/bsdiff_apply.cc

Status: Fixed (was: Started)
No more is it unsandboxed, the DCHECK should no longer fire.
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32afc44b332255130c247e3bfc4a27261d11ddfb

commit 32afc44b332255130c247e3bfc4a27261d11ddfb
Author: noel <noel@chromium.org>
Date: Fri Dec 23 08:28:46 2016

Convert utility process out-of-process file patching to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send().

Add a in-process browser test for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file matches the golden output file.

Delete all the IPC related to out-of-process file patching since we
no longer use them, we use chrome::mojom::FilePatcher instead.

[1] chrome_content_utility_manifest_overlay.json

TBR=waffles@chromium.org
BUG= 597124 ,  660325 

Review-Url: https://codereview.chromium.org/2566053002
Cr-Commit-Position: refs/heads/master@{#440601}

[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/browser/chrome_content_utility_manifest_overlay.json
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/browser/component_updater/component_patcher_operation_out_of_process.h
[add] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/common/BUILD.gn
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/common/chrome_utility_messages.h
[add] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/common/file_patcher.mojom
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/test/BUILD.gn
[modify] https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb/chrome/utility/chrome_content_utility_client.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53919401199f8cc8c0e5042dd9c0fc9716825a1c

commit 53919401199f8cc8c0e5042dd9c0fc9716825a1c
Author: wfh <wfh@chromium.org>
Date: Sun Dec 25 21:30:26 2016

Revert of Convert utility process out-of-process file patching to mojo (patchset #15 id:420001 of https://codereview.chromium.org/2566053002/ )

Reason for revert:
broke component updater - see  crbug.com/676960 

Original issue's description:
> Convert utility process out-of-process file patching to mojo
>
> Define interface chrome::mojom::FilePatcher for out-of-process file
> patching. Add that interface to the utility process, and expose the
> interface to the browser process by mojo policy [1].
>
> ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
> it with a content::UtilityProcessMojoClient() and use that to start
> and stop the utility process needed for out-of-process file patches
> and to perform out-of-process file patches using mojo calls, rather
> than IPC::Send().
>
> Add a in-process browser test for the ChromeOutOfProcessPatcher (it
> had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
> to random locations (input_file, patch_file) and patch them: checks
> the patched output_file matches the golden output file.
>
> Delete all the IPC related to out-of-process file patching since we
> no longer use them, we use chrome::mojom::FilePatcher instead.
>
> [1] chrome_content_utility_manifest_overlay.json
>
> TBR=waffles@chromium.org
> BUG= 597124 ,  660325 
>
> Committed: https://crrev.com/32afc44b332255130c247e3bfc4a27261d11ddfb
> Cr-Commit-Position: refs/heads/master@{#440601}

TBR=jochen@chromium.org,dcheng@chromium.org,sammc@chromium.org,tibell@chromium.org,waffles@chromium.org,sorin@chromium.org,noel@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 597124 ,  660325 , 676960 

Review-Url: https://codereview.chromium.org/2599393002
Cr-Commit-Position: refs/heads/master@{#440686}

[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/browser/chrome_content_utility_manifest_overlay.json
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/browser/component_updater/component_patcher_operation_out_of_process.h
[delete] https://crrev.com/79c889974fa1506fa36408f2bf4538f4a7ca70cf/chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/common/BUILD.gn
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/common/chrome_utility_messages.h
[delete] https://crrev.com/79c889974fa1506fa36408f2bf4538f4a7ca70cf/chrome/common/file_patcher.mojom
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/test/BUILD.gn
[modify] https://crrev.com/53919401199f8cc8c0e5042dd9c0fc9716825a1c/chrome/utility/chrome_content_utility_client.cc

Sign in to add a comment