Accessing an invalid iterator can sometimes be a security
issue and these checks are cheap, so upgrade to CHECKs.
Generally these DCHECKS precede a use or erase of the
checked iterator, which if the check is invalid (ie. the
iterator == .end()) is UB.
Added checks are NotFatalUntil::M130.
`base/not_fatal_until.h` is added using tools/add_header.py,
this may result in some main-file (foo.h for foo.cc) headers
being re-sorted to be first as part `git cl format` of this CL.
For this CL instances were located with a weggli query:
```
weggli --verbose=1 --cpp \
'DCHECK(_ != _.end());' \
-p 'DCHECK(_.end() != _);' \
-p 'DCHECK_NE(_, _.end());' \
-p 'DCHECK_NE(_.end(), _);'
```
which should avoid any potentially expensive calculations
of the thing to be matched against .end().
This CL was uploaded by git cl split.
R=alexmos@chromium.org, dom@chromium.org, jinsukkim@chromium.org, peter@chromium.org, wanderview@chromium.org
Bug: 351745839
Change-Id: Ic4b66209052fde03394b9f34241ae2bd9173ed7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5706540
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Auto-Submit: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1327876}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ibc66b8c440e4bcdef414e77fef4d9874d2ea9951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5493800
Auto-Submit: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293330}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ieeb461e2d489e86fd50b87a2a0721a2be34520c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5467317
Owners-Override: Alison Gale <agale@chromium.org>
Commit-Queue: Darryl James <dljames@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Darryl James <dljames@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290198}
This CL removes the PinUser32 helper functionality added to utility
processes as part of this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/4534183
The following CL removes the callers of user32 from FileUtilService,
the only thing that depended on PinUser32:
https://chromium-review.googlesource.com/c/chromium/src/+/5397955
As the function isn't needed anymore, it should be safe to remove the
code. The feature is also incompatible with the WinSboxNoFakeGdiInit
feature, which prevents user32.dll from loading in kService sandbox.
How validated:
Now that browser_tests.exe delayloads similar to chrome.exe, the
following tests actually validate FileUtilService functionality
that used to depend on User32.dll:
DownloadProtectionServiceBrowserTest.MultipartRarInspection
DownloadProtectionServiceBrowserTest.MultipartRarInspectionSecondPart
DownloadProtectionServiceBrowserTest.VerifyRarHash
Bug: 326277735
Change-Id: I1a38dd34243e75103e8696ce6bf9eaf31f425d7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5408185
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Stefan Smolen <ssmole@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1281902}
This is a reland of commit 96dbfbb77a
bool fields changed to std::optional<> to ensure they are initialized
in all cases. Fixes failures on msan builds.
Original change's description:
> Guard binding viz.mojom.gpu in utilities
>
> Adds WithGpuClient() option and associated passkeys to the service
> process host so that only allowed utilities can access the gpu
> service by directly binding viz.mojom.gpu.
>
> Bug: 328099369
> Change-Id: I6561bd5f2f5ec41241c8fe2582fab83ea37d880a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5342901
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Commit-Queue: Alex Gough <ajgo@chromium.org>
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1268729}
Bug: 328099369
Change-Id: I9ed8f30d28ef6b56ff27833a7df651d2599e0722
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5347643
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1269305}
Adds WithGpuClient() option and associated passkeys to the service
process host so that only allowed utilities can access the gpu
service by directly binding viz.mojom.gpu.
Bug: 328099369
Change-Id: I6561bd5f2f5ec41241c8fe2582fab83ea37d880a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5342901
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1268729}
CreateProcessHostForBuiltinServiceInstance is only called for
ExecutionMode::kOutOfProcessBuiltin (see
services/service_manager/service_manager.cc). Cast never uses that mode;
there are no instances of kOutOfProcessBuiltin being used in chromium or
in the internal cast codebase.
Bug: 1328879, 977637
Change-Id: I23ef669290c860d6d976f9d21fcd351f62ba6f0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5254861
Reviewed-by: Vigen Issahhanjan <vigeni@google.com>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Antonio Rivera <antoniori@google.com>
Cr-Commit-Position: refs/heads/main@{#1256846}
This CL adds a service process host option to pin user32
prior to lockdown in utility main, and plumbs the option
from the service process host.
Echo service tests are added to exercise the function
in user32 which motivated user32 pinning for services.
As content_browsertests effectively pins user32 anyway
this test always passes, but it will prevent regressions.
Services will migrate to use this API in a following CL.
Bug: 1435571,1408988
Change-Id: Ie36f16987f05cdcf42c70d28afbb04f6678c1e5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4534183
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1146962}
This is a reland of commit fe96e5d8bd
Now builds correctly for asan.
Additionally, during testing a flaky failure was uncovered where
the mojo service could start before the preloading happened, this
is resolved by moving the preloading code ahead of IO thread
creation in UtilityMain.
Original change's description:
> Allow utilities to preload libraries before sandbox lockdown
>
> Some utilities on Windows wrap dynamic libraries which might be
> distributed as components. Allowing these to load requires punching
> holes in the sandbox using file interceptions that are being phased
> out.
>
> This CL introduces a mechanism for service processes to specify a
> list of dll paths which will be loaded before the sandbox goes into
> lockdown. They can later be loaded by code using the same path, as
> Windows keeps them in memory.
>
> The mechanism uses a new opaque data transfer mechanism in the
> sandbox which will extend to different uses later. For
> now the list of DLLs is the only data transferred so this CL uses
> base::Pickle to serialize & transfer the DLLs to load.
>
> As loading arbitrary DLLs is a powerful capability a passkey is
> added for the new service process host option, and a file containing
> valid callers is added with OWNERS from the sandbox to ensure
> security review.
>
> A following CL will enable this behavior for screen_ai, but at this
> point only tests exercise these APIs
>
> The flow is roughly:
> ---
> In the browser/broker:-
>
> ServiceProcessHost gets a new option:
> .WithPreloadedLibraries
>
> UtilityProcessHost gets a new member:
> preload_libraries_
>
> UtilityProcessHost gives this to the UtilitySandboxedProcessLauncherDelegate
>
> UtilitySandboxedProcessLauncherDelegate turns a list of DLLs into an
> opaque blob and attaches it to the sandbox's TargetPolicy during
> PreSpawnTarget.
>
> The sandbox smuggles this blob into the target (previous CL).
>
> In the child/target:-
>
> The UtilityMain knows it might have a blob, so asks TargetServices.
>
> UtilityMain coordinates with its sandbox delegate, so knows that
> the blob is a pickled list of DLLs.
>
> UtilityMain loads these dlls before sandbox lockdown.
> ---
>
> Bug: 1435571
> Change-Id: Ided63f9d723811b66c183335b5533c84e9783a2a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4471953
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Ken Rockot <rockot@google.com>
> Reviewed-by: Will Harris <wfh@chromium.org>
> Commit-Queue: Will Harris <wfh@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1137415}
Bug: 1435571
Change-Id: Id0156d4649efd67adc5fe2eadaa73415b9c35503
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518747
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1143553}
This reverts commit fe96e5d8bd.
Reason for revert: This appears to be the cause of a build failure on win-asan that is closing the tree:
ci.chromium.org/ui/p/chromium/builders/ci/win-asan/26430/overview
Original change's description:
> Allow utilities to preload libraries before sandbox lockdown
>
> Some utilities on Windows wrap dynamic libraries which might be
> distributed as components. Allowing these to load requires punching
> holes in the sandbox using file interceptions that are being phased
> out.
>
> This CL introduces a mechanism for service processes to specify a
> list of dll paths which will be loaded before the sandbox goes into
> lockdown. They can later be loaded by code using the same path, as
> Windows keeps them in memory.
>
> The mechanism uses a new opaque data transfer mechanism in the
> sandbox which will extend to different uses later. For
> now the list of DLLs is the only data transferred so this CL uses
> base::Pickle to serialize & transfer the DLLs to load.
>
> As loading arbitrary DLLs is a powerful capability a passkey is
> added for the new service process host option, and a file containing
> valid callers is added with OWNERS from the sandbox to ensure
> security review.
>
> A following CL will enable this behavior for screen_ai, but at this
> point only tests exercise these APIs
>
> The flow is roughly:
> ---
> In the browser/broker:-
>
> ServiceProcessHost gets a new option:
> .WithPreloadedLibraries
>
> UtilityProcessHost gets a new member:
> preload_libraries_
>
> UtilityProcessHost gives this to the UtilitySandboxedProcessLauncherDelegate
>
> UtilitySandboxedProcessLauncherDelegate turns a list of DLLs into an
> opaque blob and attaches it to the sandbox's TargetPolicy during
> PreSpawnTarget.
>
> The sandbox smuggles this blob into the target (previous CL).
>
> In the child/target:-
>
> The UtilityMain knows it might have a blob, so asks TargetServices.
>
> UtilityMain coordinates with its sandbox delegate, so knows that
> the blob is a pickled list of DLLs.
>
> UtilityMain loads these dlls before sandbox lockdown.
> ---
>
> Bug: 1435571
> Change-Id: Ided63f9d723811b66c183335b5533c84e9783a2a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4471953
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Ken Rockot <rockot@google.com>
> Reviewed-by: Will Harris <wfh@chromium.org>
> Commit-Queue: Will Harris <wfh@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1137415}
Bug: 1435571
Change-Id: I8b3fd2ae342a173c532da08f1015266cf5d7b64c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4492274
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Michael Wilson <mjwilson@chromium.org>
Commit-Queue: Michael Wilson <mjwilson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1137435}
Some utilities on Windows wrap dynamic libraries which might be
distributed as components. Allowing these to load requires punching
holes in the sandbox using file interceptions that are being phased
out.
This CL introduces a mechanism for service processes to specify a
list of dll paths which will be loaded before the sandbox goes into
lockdown. They can later be loaded by code using the same path, as
Windows keeps them in memory.
The mechanism uses a new opaque data transfer mechanism in the
sandbox which will extend to different uses later. For
now the list of DLLs is the only data transferred so this CL uses
base::Pickle to serialize & transfer the DLLs to load.
As loading arbitrary DLLs is a powerful capability a passkey is
added for the new service process host option, and a file containing
valid callers is added with OWNERS from the sandbox to ensure
security review.
A following CL will enable this behavior for screen_ai, but at this
point only tests exercise these APIs
The flow is roughly:
---
In the browser/broker:-
ServiceProcessHost gets a new option:
.WithPreloadedLibraries
UtilityProcessHost gets a new member:
preload_libraries_
UtilityProcessHost gives this to the UtilitySandboxedProcessLauncherDelegate
UtilitySandboxedProcessLauncherDelegate turns a list of DLLs into an
opaque blob and attaches it to the sandbox's TargetPolicy during
PreSpawnTarget.
The sandbox smuggles this blob into the target (previous CL).
In the child/target:-
The UtilityMain knows it might have a blob, so asks TargetServices.
UtilityMain coordinates with its sandbox delegate, so knows that
the blob is a pickled list of DLLs.
UtilityMain loads these dlls before sandbox lockdown.
---
Bug: 1435571
Change-Id: Ided63f9d723811b66c183335b5533c84e9783a2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4471953
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1137415}
Some service processes are per-site (e.g. MediaFoundationService). When
the process crashes, it's important to know which site caused the crash,
e.g. it could be related to the video stream that the site uses.
This CL adds site info to ServiceProcessInfo and related classes, such
that when a service process crashes, the observer can get the site info.
Also updated MediaFoundationServiceMonitor to log the site when crash
happens. In a later CL, we'll use this info to implement per-site
fallback logic.
Manually tested by triggering MediaFoundationService crash in task
manager and saw the log in MediaFoundationServiceMonitor:
```
OnServiceProcessCrashed: MediaFoundationService process for
https://widevine.com/ crashed!
```
Bug: b/233125572
Test: Added browser test. See above for manual test done.
Change-Id: I232c127954b8fef27d34d858ecc3ff6864125f9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4218910
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1102241}
Turns out there's a lot of includes, so these will have to be removed
before deleting the implementation of the task runner handles.
To allow the deletion of the task runner handle headers, add
the sequenced/thread task runner handles where they are used in
the codebase with scripts.
This was done with an automated change, with a few touchups afterwards.
The code for the mass-refactor changes are here:
python:
https://paste.googleplex.com/5534570878337024
shell:
https://paste.googleplex.com/6466750748033024
In terms of touchups:
- add sequenced/thread task runner handles to
the third_party/blink/public/DEPS, because multiple files were using
it transitively anyways.
- rewrite certain parts of the codebase which used
ThreadTaskRunnerHandles instead of CurrentDefaultHandles.
- fix a compile issue with forward-declaration in
extensions/browser/extension_file_task_runner.h.
AX-Relnotes: n/a.
Bug: 1026641
Change-Id: I737ef32aee4e77c21eaa3a2bdc403a28322cf1b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4133323
Owners-Override: Gabriel Charette <gab@chromium.org>
Commit-Queue: Sean Maher <spvw@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1090532}
It is possible that the process id might have already been re-used by
the time that the value is checked. This old behavior is therefore
considered unsafe.
This CL changes ServiceProcessInfo to use a base::Process similar to
the existing ChildProcessData struct.
This CL updates all callers to try and preserve the base::Process
until the Pid is needed, and adds checks for instances where the
base::Process might have already terminated before the Pid is needed.
BUG=None
Change-Id: I9871bf91bbab6f1fecd082b20fb8d1a81abf9833
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3783058
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028009}
Remove the exceptions for is_chromecast on Fuchsia within the Content
layer, including //media, and Viz. Now that Content is no longer built
as part of the //chromecast:cast_test_lists target on Fuchsia, these
files are no longer reached, and the exceptions can be removed.
Also remove a similar exception in Chrome tests and update a couple TODOs to correctly reference https://crbug.com/1329657.
Bug: 1330636, 1329657
Test: gn check, ninja chromecast:cast_test_lists
Change-Id: If9b037d5b17b31183a2f1546bae817908c03279f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3712362
Reviewed-by: Will Cassella <cassew@chromium.org>
Commit-Queue: David Dorwin <ddorwin@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1018918}
This deprecated API and associate code are only used as Chromecast. Mark
it as Chromecast-only to reduce build cost on other platforms, and to
make it harder to add new callers.
Bug: 977637
Change-Id: I15260e371938e62fddc598c96d847ee37630e95d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3309879
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#947087}
sandbox.mojom.Sandbox can now be used as the sandbox type so we
replace it everywhere.
The guts of the change are in //sandbox/policy/sandbox_type.h where
SandboxType is now deleted, and //sandbox/policy/mojom/sandbox.mojom
where sandbox types that are not already used in mojom ServiceSandbox
attributes are added.
Some cascading changes:-
- kService wasn't implemented on Mac (as it is equivalent to kUtility).
As we cannot alias enum fields in mojo like we can in C++ I have added
kService for Mac. The alternative is to define platform specific
ServiceSandbox attributes for all kService interfaces which seems to
put this complexity in the wrong place.
- sandbox_type.h included a number of buildflag headers that other files
then relied on. As sandbox_type.h is no longer needed in many places
and no longer needs these defines, they have been introduced where
required.
- sandbox::mojom::Sandbox is forward declared in a couple of headers
that are widely imported, hopefully reducing the number of times the
mojom.h is included but not used.
- some build deps must be modified.
- LibAssistantService needs a sandbox to be defined even when hosted
in process, so has kNoSandbox now when enable_cros_libassistant is
false.
Bug: 1210301
Change-Id: I13fa4fa8cbbb3090a38806fe5532787bbdf1e2fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3213677
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#934126}
Adds ServiceSandbox attribute to AudioService and NetworkService
interfaces and deletes the final service_sandbox_type.h file. All
service sandboxes are now specified this way.
These two services are different, however. Their sandboxes can be
disabled (to kNoSandbox) by policy or feature flags. This means
the service launching machinery must know how to do this. In this
CL we move this logic into the ServiceProcessHost which in turn
asks the ContentBrowserClient if the sandboxes should be enabled.
As this is only necessary for two sandbox types we have not
generalised the CBC interface.
We also delete `sandbox_type` from service process host options
as it cannot be specified as an option and must be specified using
the ServiceSandbox attribute on the launched mojom. We migrate
the type used within ServiceProcessHost to sandbox::mojom::Sandbox.
A future CL will replace sandbox::policy::mojom::SandboxType with
the mojom type throughout.
Bug: 1210301
Change-Id: I4de13359487158a52da8ad414c8beb9cd93033bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3209992
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#930365}
This replaces DISALLOW_COPY_AND_ASSIGN with explicit constructor deletes
where a local script is able to detect its insertion place (~Foo() is
public => insert before this line).
This is incomplete as not all classes have a public ~Foo() declared, so
not all DISALLOW_COPY_AND_ASSIGN occurrences are replaced.
IWYU cleanup is left as a separate pass that is easier when these macros
go away.
Bug: 1010217
Change-Id: Iea478401b7580682c7b9f195f7af9cbbdb6ce315
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3167292
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923194}
They were on the IO thread to avoid deadlocks with NPAPI plugins, but we don't have that anymore.
Currently this is off by default behind a feature flag. I have tried to minimize the churn in the code to enable this. Some places where it would involve too much duplication or refactoring I've kept with PostTasks from UI to UI thread. Once this is enabled by default then I'll simplify all the callsites.
Bug: 904556
Change-Id: Ib664d644e0af016d5736348218bcc38cb79341f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2806425
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Owners-Override: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#870614}
Note to QA: This CL is purely mechanical and shouldn't be blamed
for future regressions on touched files.
This is a follow-up to https://chromium-review.googlesource.com/c/chromium/src/+/2211138
which already removed all usage using content::BrowserThread.
Hence this script now matches unqualified BrowserThread:: without
risking having "content::" be selected as "traits_before" by the regex
(ran on same revision as step #1).
content:: is now always added if outside namespace content {}
(deleting unused using content::BrowserThread; decls)
Script @ https://crbug.com/1026641#c92
(will TBR fdoray@ post-review for mechanical change)
TBR=fdoray@hchromium.org
AX-Relnotes: n/a.
Bug: 1026641
Change-Id: I51ae2f83eb17d19b54563fd9b4fc040d2aa0c948
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2212469
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772458}
This is a reland of e8d24f737e
Original change's description:
> Reland "Move Audio Service off Service Manager"
>
> This is a reland of a4d27f513e
>
> The reland moves the TestingApi binder support into a component
> library to avoid duplicate definitions that resulted in browser
> test failures in component builds.
>
> Original change's description:
> > Move Audio Service off Service Manager
> >
> > All the details around if/when a service instance is started and whether
> > it's in- or out-of-process have been centralized into the implementation
> > of a single GetAudioService() helper in Content.
> >
> > A few other helpers are added to ease the transition off the Service
> > Manager, though they aren't necessarily ideal APIs. For example,
> > client library APIs which once took a Service Manager Connector may now
> > take a RepeatingCallback to bind a specific type of interface.
> >
> > In addition to GetAudioService(), Content provides helpers for
> > AudioSystem construction and StreamFactory binding from any sequence.
> >
> > Because all timeout/lifetime management logic is now generically
> > supported within Mojo and Content and requires no service implementation
> > details to get right (and because unit tests launching service processes
> > is no longer supported outside of Content), all Audio Service test
> > coverage relevant to lifetime management is deemed superfluous and has
> > been removed.
> >
> > Bug: 977637
> > Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
> > Tbr: oshima@chromium.org
> > Tbr: guidou@chromium.org
> > Tbr: vollick@chromium.org
> > Tbr: xiaohuic@chromium.org
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
> > Commit-Queue: Ken Rockot <rockot@google.com>
> > Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Reviewed-by: Robert Sesek <rsesek@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#721832}
>
> Bug: 977637
> Change-Id: I183e79042c99458f2abf9423d8f9bbefedf5b374
> No-Presubmit: true
> Tbr: oshima@chromium.org
> Tbr: guidou@chromium.org
> Tbr: vollick@chromium.org
> Tbr: xiaohuic@chromium.org
> Tbr: avi@chromium.org
> Tbr: rsesek@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952601
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/master@{#722373}
Bug: 977637
Change-Id: I1b803e57632bd92f2b1e093c3d1c1731de8338f0
No-Presubmit: true
Tbr: oshima@chromium.org
Tbr: guidou@chromium.org
Tbr: vollick@chromium.org
Tbr: xiaohuic@chromium.org
Tbr: avi@chromium.org
Tbr: rsesek@chromium.org
Tbr: dalecurtis@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1954772
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#722428}
This reverts commit e8d24f737e.
Reason for revert:
Findit (https://goo.gl/kROfz5) identified CL at revision 722373 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2U4ZDI0ZjczN2UyOWYzZmJhM2U0NjU0ZWExZGMxMjU5ZTEzNzMwNTgM
Sample Failed Build: https://ci.chromium.org/b/8894850112495909312
Sample Failed Step: browser_tests
Original change's description:
> Reland "Move Audio Service off Service Manager"
>
> This is a reland of a4d27f513e
>
> The reland moves the TestingApi binder support into a component
> library to avoid duplicate definitions that resulted in browser
> test failures in component builds.
>
> Original change's description:
> > Move Audio Service off Service Manager
> >
> > All the details around if/when a service instance is started and whether
> > it's in- or out-of-process have been centralized into the implementation
> > of a single GetAudioService() helper in Content.
> >
> > A few other helpers are added to ease the transition off the Service
> > Manager, though they aren't necessarily ideal APIs. For example,
> > client library APIs which once took a Service Manager Connector may now
> > take a RepeatingCallback to bind a specific type of interface.
> >
> > In addition to GetAudioService(), Content provides helpers for
> > AudioSystem construction and StreamFactory binding from any sequence.
> >
> > Because all timeout/lifetime management logic is now generically
> > supported within Mojo and Content and requires no service implementation
> > details to get right (and because unit tests launching service processes
> > is no longer supported outside of Content), all Audio Service test
> > coverage relevant to lifetime management is deemed superfluous and has
> > been removed.
> >
> > Bug: 977637
> > Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
> > Tbr: oshima@chromium.org
> > Tbr: guidou@chromium.org
> > Tbr: vollick@chromium.org
> > Tbr: xiaohuic@chromium.org
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
> > Commit-Queue: Ken Rockot <rockot@google.com>
> > Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Reviewed-by: Robert Sesek <rsesek@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#721832}
>
> Bug: 977637
> Change-Id: I183e79042c99458f2abf9423d8f9bbefedf5b374
> No-Presubmit: true
> Tbr: oshima@chromium.org
> Tbr: guidou@chromium.org
> Tbr: vollick@chromium.org
> Tbr: xiaohuic@chromium.org
> Tbr: avi@chromium.org
> Tbr: rsesek@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952601
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/master@{#722373}
Change-Id: I39c5079ef87d681d2854314e82195ec203498929
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 977637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1954771
Cr-Commit-Position: refs/heads/master@{#722398}
This is a reland of a4d27f513e
The reland moves the TestingApi binder support into a component
library to avoid duplicate definitions that resulted in browser
test failures in component builds.
Original change's description:
> Move Audio Service off Service Manager
>
> All the details around if/when a service instance is started and whether
> it's in- or out-of-process have been centralized into the implementation
> of a single GetAudioService() helper in Content.
>
> A few other helpers are added to ease the transition off the Service
> Manager, though they aren't necessarily ideal APIs. For example,
> client library APIs which once took a Service Manager Connector may now
> take a RepeatingCallback to bind a specific type of interface.
>
> In addition to GetAudioService(), Content provides helpers for
> AudioSystem construction and StreamFactory binding from any sequence.
>
> Because all timeout/lifetime management logic is now generically
> supported within Mojo and Content and requires no service implementation
> details to get right (and because unit tests launching service processes
> is no longer supported outside of Content), all Audio Service test
> coverage relevant to lifetime management is deemed superfluous and has
> been removed.
>
> Bug: 977637
> Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
> Tbr: oshima@chromium.org
> Tbr: guidou@chromium.org
> Tbr: vollick@chromium.org
> Tbr: xiaohuic@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
> Commit-Queue: Ken Rockot <rockot@google.com>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#721832}
Bug: 977637
Change-Id: I183e79042c99458f2abf9423d8f9bbefedf5b374
No-Presubmit: true
Tbr: oshima@chromium.org
Tbr: guidou@chromium.org
Tbr: vollick@chromium.org
Tbr: xiaohuic@chromium.org
Tbr: avi@chromium.org
Tbr: rsesek@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952601
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#722373}
This reverts commit a4d27f513e.
Reason for revert:
Findit (https://goo.gl/kROfz5) identified CL at revision 721832 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2E0ZDI3ZjUxM2ViNDBhODk1MDVmMDQyZDY4NjkzZWQ2NzAyNDAwZDkM
Sample Failed Build: https://ci.chromium.org/b/8894942016495385648
Sample Failed Step: browser_tests
Original change's description:
> Move Audio Service off Service Manager
>
> All the details around if/when a service instance is started and whether
> it's in- or out-of-process have been centralized into the implementation
> of a single GetAudioService() helper in Content.
>
> A few other helpers are added to ease the transition off the Service
> Manager, though they aren't necessarily ideal APIs. For example,
> client library APIs which once took a Service Manager Connector may now
> take a RepeatingCallback to bind a specific type of interface.
>
> In addition to GetAudioService(), Content provides helpers for
> AudioSystem construction and StreamFactory binding from any sequence.
>
> Because all timeout/lifetime management logic is now generically
> supported within Mojo and Content and requires no service implementation
> details to get right (and because unit tests launching service processes
> is no longer supported outside of Content), all Audio Service test
> coverage relevant to lifetime management is deemed superfluous and has
> been removed.
>
> Bug: 977637
> Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
> Tbr: oshima@chromium.org
> Tbr: guidou@chromium.org
> Tbr: vollick@chromium.org
> Tbr: xiaohuic@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
> Commit-Queue: Ken Rockot <rockot@google.com>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#721832}
Change-Id: Ic59416fc7c5f5eda2c3b8b211773f9761d2ea03d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 977637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952526
Cr-Commit-Position: refs/heads/master@{#721896}
All the details around if/when a service instance is started and whether
it's in- or out-of-process have been centralized into the implementation
of a single GetAudioService() helper in Content.
A few other helpers are added to ease the transition off the Service
Manager, though they aren't necessarily ideal APIs. For example,
client library APIs which once took a Service Manager Connector may now
take a RepeatingCallback to bind a specific type of interface.
In addition to GetAudioService(), Content provides helpers for
AudioSystem construction and StreamFactory binding from any sequence.
Because all timeout/lifetime management logic is now generically
supported within Mojo and Content and requires no service implementation
details to get right (and because unit tests launching service processes
is no longer supported outside of Content), all Audio Service test
coverage relevant to lifetime management is deemed superfluous and has
been removed.
Bug: 977637
Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
Tbr: oshima@chromium.org
Tbr: guidou@chromium.org
Tbr: vollick@chromium.org
Tbr: xiaohuic@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721832}
On Android we can somehow observe utility process crashes without
observing process launches. This is a simple fix to avoid memory bugs in
that case.
Also fixes a related leak in the internal ServiceProcessTracker, where
we were never cleaning up ServiceProcessInfo entries after process
termination.
Bug: 1016027
Change-Id: Ia5a8df891547cb7a2f01c869d16b014a8b249423
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894627
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#711829}
*** Note: There is no behavior change from this patch. ***
The PostTask APIs will shortly be changed to require all tasks to explicitly
specify their thread affinity, i.e., whether the task should run on the thread
pool or a specific named thread such as a BrowserThread. This patch updates all
call sites with thread affinity annotation. We also remove the "WithTraits"
suffix to make the call sites more readable.
Before:
// Thread pool task.
base::PostTaskWithTraits(FROM_HERE, {...}, ...);
// UI thread task.
base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI, ...}, ...);
After:
// Thread pool task.
base::PostTask(FROM_HERE, {base::ThreadPool(), ...}, ...);
// UI thread task.
base::PostTask(FROM_HERE, {BrowserThread::UI, ...}, ...);
This patch was semi-automatically prepared with these steps:
1. Patch in https://chromium-review.googlesource.com/c/chromium/src/+/1635827
to make thread affinity a build-time requirement.
2. Run an initial pass with a clang rewriter:
https://chromium-review.googlesource.com/c/chromium/src/+/1635623
3. ninja -C out/Debug | grep 'requested here' | cut -d: -f1-3 | sort | \
uniq > errors.txt
4. while read line; do
f=$(echo $line | cut -d: -f 1)
r=$(echo $line | cut -d: -f 2)
c=$(echo $line | cut -d: -f 3)
sed -i "${r}s/./&base::ThreadPool(),/$c" $f
done < errors.txt
5. GOTO 3 until build succeeds.
6. Remove the "WithTraits" suffix from task API call sites:
$ tools/git/mffr.py -i <(cat <<EOF
[
["PostTaskWithTraits", "PostTask"],
["PostDelayedTaskWithTraits", "PostDelayedTask"],
["PostTaskWithTraitsAndReply", "PostTaskAndReply"],
["CreateTaskRunnerWithTraits", "CreateTaskRunner"],
["CreateSequencedTaskRunnerWithTraits", "CreateSequencedTaskRunner"],
["CreateUpdateableSequencedTaskRunnerWithTraits", "CreateUpdateableSequencedTaskRunner"],
["CreateSingleThreadTaskRunnerWithTraits", "CreateSingleThreadTaskRunner"],
["CreateCOMSTATaskRunnerWithTraits", "CreateCOMSTATaskRunner"]
]
EOF
)
This CL was uploaded by git cl split.
R=boliu@chromium.org, tsepez@chromium.org
Bug: 968047
Change-Id: I346372d16a3856186ea74d14e0dd8a12f7cacae5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729589
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Auto-Submit: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683554}
This refactors the Video Capture service and its clients to use the
simpler service model described in
https://docs.google.com/document/d/1M0-K0gi1xXO0f_-YKSH2LFVh4RJY-xe9T9VaGFOSXb0/edit.
The DeviceFactoryProvider interface is renamed to VideoCaptureService
and previous binders for other top-level interfaces (TestingControls and
cros.mojom.CrosImageCapture) have been added to the interface.
Service lifetime is no longer managed by manual ref-count but is instead
automated using new Mojo interface idling bevhaior. The behavior should
effectively mirror the service's behavior before this change. Namely:
- The service stays alive as long as any receivers are bound other
than the main interface pipe (previously DeviceFactoryProvider, now
VideoCaptureService). Once this condition is no longer met, the
service is considered idle.
- On non-Android platforms, when out-of-process Video Capture is
enabled, the service process is killed after 5 seconds of continuous
idling.
- On Android, the service process does not tear itself down ever.
- If out-of-process Video Capture is not enabled, all clients will use
the same shared in-browser instance of the service, which lives
forever.
This also ends up deleting a bunch of tests for the service which
effectively tested behavior that's no longer applicable, like lifetime
behavior built around VideoCaptureService (nee DeviceFactoryProvider)
connection.
Bug: 977637
Change-Id: I96ccf3558be7d173041de0de70251d841dd519f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710051
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#680168}
ServiceProcessHost::Options allows the user to set some flags to
configure the child process, but these were not being passed to the
corresponding UtilityProcessHost as intended. This fixes that.
Bug: 986183
Change-Id: I19fd3846032a23b1a6498792b0369d21738a4008
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1713294
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#679674}
This change covers a few useful improvements to ServiceProcessHost.
First, there are no longer actual ServiceProcessHost object
instances for users to retain. Instead, service process lifetime
is tied strictly to the lifetime of the relevant service pipe.
Either the service or the browser side can hang up to elicit
service process teardown.
Second, API surface has been added to allow callers to enumerate
currently running service processes as well as observe service
process lifecycle events globally.
ServiceProcessHost tests are updated to cover the new usage, as
well as to demonstrate recommended usage in conjunction with new
Mojo Remote idle timeout behavior for managing service process
lifetime.
Finally, additional documentation and clarification has been added
to ServiceProcessHost's header.
This is part of the ongoing effort to simplify Chrome services:
https://docs.google.com/document/d/1M0-K0gi1xXO0f_-YKSH2LFVh4RJY-xe9T9VaGFOSXb0/edit
Bug: 977637
Change-Id: If789159df0fcebae5161323a2a323481005c9516
Tbr: dcheng@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703095
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678705}
ServiceProcessHost allows Content and its embedders to launch a new
service process for any given mojom service interface.
This CL wires up the support and converts a single test-only service
to use ServiceProcessHost instead of integrating with Service Manager.
Bug: 977637
Change-Id: I63e846bf816c51ba2c557bfdc8b41189a92d2890
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1673922
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#672676}
