[Reland] Use a separate utility process for system proxy resolution for Mac [chromium/src : main]
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
Hayato ItoRelanding the CL: https://chromium-review.googlesource.com/c/chromium/src/+/7761426
It was previously reverted due to failures caused by newly added tests in the `Mac-12 tests` and `Mac 15 rel tests` builds. These tests & builds now appear to be passing, so no further action is required for those specific failures.
Mayur PatilThanks for the update. To help with the review, could you clarify the following?
- Is this CL identical to the reverted one, or does it include any fixes/modifications?
- What was the exact root cause of the previous failures on the Mac bots?
I would appreciate it if you update the CL description so the description explains why this reland is safe. Thanks!
This reland is not bit-for-bit identical to the reverted CL, but the changes from the reverted patch are non-functional cleanup only: comment/formatting updates (added closure comments at end of preprocesser directives `#endif`) and an include cleanup(removed `#include<string>`). It does not contain a behavioral fix for the previous Mac failures.
Related details
- Hayato Ito
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito added 1 comment![]( "Open in Gerrit")
Hayato ItoRelanding the CL: https://chromium-review.googlesource.com/c/chromium/src/+/7761426
It was previously reverted due to failures caused by newly added tests in the `Mac-12 tests` and `Mac 15 rel tests` builds. These tests & builds now appear to be passing, so no further action is required for those specific failures.
Mayur PatilThanks for the update. To help with the review, could you clarify the following?
- Is this CL identical to the reverted one, or does it include any fixes/modifications?
- What was the exact root cause of the previous failures on the Mac bots?
I would appreciate it if you update the CL description so the description explains why this reland is safe. Thanks!
Hayato ItoThis reland is not bit-for-bit identical to the reverted CL, but the changes from the reverted patch are non-functional cleanup only: comment/formatting updates (added closure comments at end of preprocesser directives `#endif`) and an include cleanup(removed `#include<string>`). It does not contain a behavioral fix for the previous Mac failures.
Thanks for the explanation.
Just FYI.
https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#relanding-a-change
Describe the fix: In the commit message of the reland change, briefly summarize what's changed that makes relanding again safe. Explanations can include: “included needed fix”, “disabled failing tests”, “crash was fixed elsewhere”. Specifically for that last case: if the reland change is identical to the original and the reland fix was handled separately in a preceding change, make sure to link to that change in the commit message of the reland.
So in the case of “crash was fixed elsewhere” (my guess), briefly summarize that in your CL's description. It's a bit unclear how timeout/failure wouldn't happen in this reland.
Related details
- Kenichi Ishibashi
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
Hayato ItoRelanding the CL: https://chromium-review.googlesource.com/c/chromium/src/+/7761426
It was previously reverted due to failures caused by newly added tests in the `Mac-12 tests` and `Mac 15 rel tests` builds. These tests & builds now appear to be passing, so no further action is required for those specific failures.
Mayur PatilThanks for the update. To help with the review, could you clarify the following?
- Is this CL identical to the reverted one, or does it include any fixes/modifications?
- What was the exact root cause of the previous failures on the Mac bots?
I would appreciate it if you update the CL description so the description explains why this reland is safe. Thanks!
Hayato ItoThis reland is not bit-for-bit identical to the reverted CL, but the changes from the reverted patch are non-functional cleanup only: comment/formatting updates (added closure comments at end of preprocesser directives `#endif`) and an include cleanup(removed `#include<string>`). It does not contain a behavioral fix for the previous Mac failures.
Thanks for the explanation.
Just FYI.
https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#relanding-a-changeDescribe the fix: In the commit message of the reland change, briefly summarize what's changed that makes relanding again safe. Explanations can include: “included needed fix”, “disabled failing tests”, “crash was fixed elsewhere”. Specifically for that last case: if the reland change is identical to the original and the reland fix was handled separately in a preceding change, make sure to link to that change in the commit message of the reland.
So in the case of “crash was fixed elsewhere” (my guess), briefly summarize that in your CL's description. It's a bit unclear how timeout/failure wouldn't happen in this reland.
Thanks for the pointer.
That matches my understanding, but I do not have the links to the CL's which potentially have resolved the issue which could be claimed in the description.
So, the safety signal in this scenario is that the exact CI-only Mac browser_tests builders that caught the original timeout/sandbox-init failure have now been explicitly included on this Reland and are passing, so the previous failure is not reproducing.
I can just add a safety note in the description.
Let me know your thoughts on this?
Related details
- Hayato Ito
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hayato Ito
| Code-Review | +1 |
Hayato ItoRelanding the CL: https://chromium-review.googlesource.com/c/chromium/src/+/7761426
It was previously reverted due to failures caused by newly added tests in the `Mac-12 tests` and `Mac 15 rel tests` builds. These tests & builds now appear to be passing, so no further action is required for those specific failures.
Mayur PatilThanks for the update. To help with the review, could you clarify the following?
- Is this CL identical to the reverted one, or does it include any fixes/modifications?
- What was the exact root cause of the previous failures on the Mac bots?
I would appreciate it if you update the CL description so the description explains why this reland is safe. Thanks!
Hayato ItoThis reland is not bit-for-bit identical to the reverted CL, but the changes from the reverted patch are non-functional cleanup only: comment/formatting updates (added closure comments at end of preprocesser directives `#endif`) and an include cleanup(removed `#include<string>`). It does not contain a behavioral fix for the previous Mac failures.
Mayur PatilThanks for the explanation.
Just FYI.
https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#relanding-a-changeDescribe the fix: In the commit message of the reland change, briefly summarize what's changed that makes relanding again safe. Explanations can include: “included needed fix”, “disabled failing tests”, “crash was fixed elsewhere”. Specifically for that last case: if the reland change is identical to the original and the reland fix was handled separately in a preceding change, make sure to link to that change in the commit message of the reland.
So in the case of “crash was fixed elsewhere” (my guess), briefly summarize that in your CL's description. It's a bit unclear how timeout/failure wouldn't happen in this reland.
Thanks for the pointer.
That matches my understanding, but I do not have the links to the CL's which potentially have resolved the issue which could be claimed in the description.
So, the safety signal in this scenario is that the exact CI-only Mac browser_tests builders that caught the original timeout/sandbox-init failure have now been explicitly included on this Reland and are passing, so the previous failure is not reproducing.
I can just add a safety note in the description.
Let me know your thoughts on this?
I can just add a safety note in the description.
That sounds good to me. Thanks!
It is completely fine that you cannot guarantee this reland is 100% safe; failures can happen at any time with any CL or reland.
I just wanted to make our intentions clear in the CL description, which will be helpful for a gardener when deciding whether or not to revert it if it is suspected of being the culprit.
Related details
- Kenichi Ishibashi
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
Drive-by comments:
Have you run these tests in your local macOS environment? Even with the current state of this CL, applying it in my local environment causes the tests to fail, similar to the failures observed on the try bots.
According to the logs in Console.app, the failure seems to occur in SandboxSerializer::ApplySerializedPolicy() [here](https://source.chromium.org/chromium/chromium/src/+/main:sandbox/mac/sandbox_serializer.cc;l=215;drc=cab606d12ed8680978e482a0cdec691c7f06f6d2):
```
error 16:57:30.743360+0900 browser_tests SandboxSerializer: Failed to initialize sandbox with source mode policy: internal-strcmp: argument 1 must be: string
```
This suggests there might be an issue with sandbox/policy/mac/proxy_resolver.sb, which was added in CL [6904119](https://crrev.com/c/6904119). It might be worth reaching out to @kpan...@microsoft.com (the author of that CL) to investigate this further.
Related details
- Kenichi Ishibashi
- Mayur Patil
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
Thanks for the looking into it.
I don’t have access to a macOS environment at the moment. I’ll arrange one and test these changes there.
Till then I will keep these changes on hold.
Related details
- Hayato Ito
- Kenichi Ishibashi
- Tsuyoshi Horo
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mayur Patil
| Commit-Queue | +1 |
I was able to reproduce this locally on mac. The issue is that `proxy_resolver.sb` expects the `SYSTEM_PROXY_NETWORK_ACCESS` parameter, but `SetupSandboxParameters()` wasn’t setting it for `kProxyResolver`. Because of this mismatch, seatbelt failed to compile the profile with error `internal-strcmp`, due to which the sandbox initialization crashed.
Related details
- Hayato Ito
- Kenichi Ishibashi
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mayur Patil
| Commit-Queue | +1 |
Done
Related details
- Hayato Ito
- Kenichi Ishibashi
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 2 comments![]( "Open in Gerrit")
Thanks for fixing the sandbox parameters. The changes in this CL look good to me. However, since I haven't followed the entire history of crbug.com/442313607, please make sure to get LGTMs from bashi@ and hayato@ as well, just in case.
Regarding my comment in mac_system_proxy_resolver_mojo_unittest.cc, it’s intended as a question for hayato@ and not meant to block the submission of this CL. If you receive LGTMs from both of them but find that an unresolved comment prevents submission, please feel free to mark it as resolved.
// This shouldn't crash and there should never be a callback to
// ProxyResolutionComplete(). Post a sentinel task and wait for it to
// ensure all previously posted tasks (including the Mojo callback) have
// had a chance to run.
bool sentinel_ran = false;
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce([](bool* flag) { *flag = true; }, &sentinel_ran));
ASSERT_TRUE(base::test::RunUntil([&]() { return sentinel_ran; }));To hay...@chromium.org: I think the current testing approach will be insufficient once the Task Scheduler is active. Do you have any ideas for a better testing strategy that accounts for prioritized task execution?
Related details
- Hayato Ito
- Kenichi Ishibashi
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito added 1 comment![]( "Open in Gerrit")
// This shouldn't crash and there should never be a callback to
// ProxyResolutionComplete(). Post a sentinel task and wait for it to
// ensure all previously posted tasks (including the Mojo callback) have
// had a chance to run.
bool sentinel_ran = false;
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce([](bool* flag) { *flag = true; }, &sentinel_ran));
ASSERT_TRUE(base::test::RunUntil([&]() { return sentinel_ran; }));To hay...@chromium.org: I think the current testing approach will be insufficient once the Task Scheduler is active. Do you have any ideas for a better testing strategy that accounts for prioritized task execution?
Please read https://chromium.googlesource.com/chromium/src/+/HEAD/net/base/task/.
Unless other post tasks explicly use lower priority task queues, base::SequencedTaskRunner::GetCurrentDefault() should work fine here in the test.
However, in general, it's not a good idea to rely on RunUntil with a sentinel; that's racy.
Having said that, if you are unsure, use RequestPriority::THROTTLED for sentinel.
e.g. net::GetTaskRunner(RequestPriority::THROTTLED)
See https://chromium-review.git.corp.google.com/c/chromium/src/+/7761426 for available RequestPriority variants.
Related details
- Kenichi Ishibashi
- Mayur Patil
Hayato Ito (Gerrit)
Hayato Ito added 1 comment![]( "Open in Gerrit")
// This shouldn't crash and there should never be a callback to
// ProxyResolutionComplete(). Post a sentinel task and wait for it to
// ensure all previously posted tasks (including the Mojo callback) have
// had a chance to run.
bool sentinel_ran = false;
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce([](bool* flag) { *flag = true; }, &sentinel_ran));
ASSERT_TRUE(base::test::RunUntil([&]() { return sentinel_ran; }));Hayato ItoTo hay...@chromium.org: I think the current testing approach will be insufficient once the Task Scheduler is active. Do you have any ideas for a better testing strategy that accounts for prioritized task execution?
Please read https://chromium.googlesource.com/chromium/src/+/HEAD/net/base/task/.
Unless other post tasks explicly use lower priority task queues, base::SequencedTaskRunner::GetCurrentDefault() should work fine here in the test.
However, in general, it's not a good idea to rely on RunUntil with a sentinel; that's racy.
Having said that, if you are unsure, use RequestPriority::THROTTLED for sentinel.
e.g. net::GetTaskRunner(RequestPriority::THROTTLED)
See https://chromium-review.git.corp.google.com/c/chromium/src/+/7761426 for available RequestPriority variants.
See https://chromium-review.git.corp.google.com/c/chromium/src/+/7761426 for available RequestPriority variants.
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
// This shouldn't crash and there should never be a callback to
// ProxyResolutionComplete(). Post a sentinel task and wait for it to
// ensure all previously posted tasks (including the Mojo callback) have
// had a chance to run.
bool sentinel_ran = false;
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce([](bool* flag) { *flag = true; }, &sentinel_ran));
ASSERT_TRUE(base::test::RunUntil([&]() { return sentinel_ran; }));Hayato ItoTo hay...@chromium.org: I think the current testing approach will be insufficient once the Task Scheduler is active. Do you have any ideas for a better testing strategy that accounts for prioritized task execution?
Hayato ItoPlease read https://chromium.googlesource.com/chromium/src/+/HEAD/net/base/task/.
Unless other post tasks explicly use lower priority task queues, base::SequencedTaskRunner::GetCurrentDefault() should work fine here in the test.
However, in general, it's not a good idea to rely on RunUntil with a sentinel; that's racy.
Having said that, if you are unsure, use RequestPriority::THROTTLED for sentinel.
e.g. net::GetTaskRunner(RequestPriority::THROTTLED)
See https://chromium-review.git.corp.google.com/c/chromium/src/+/7761426 for available RequestPriority variants.
See https://chromium-review.git.corp.google.com/c/chromium/src/+/7761426 for available RequestPriority variants.
- Hayato Ito
- Kenichi Ishibashi
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito voted Code-Review+1![]( "Open in Gerrit")
- Kenichi Ishibashi
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: aj...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): aj...@chromium.org
Reviewer source(s):
aj...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Gough
- Avi Drissman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Gough
| Code-Review | +1 |
1 comment
- Avi Drissman
- Mayur Patil
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
- Mayur Patil
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil voted Commit-Queue+2![]( "Open in Gerrit")
- Rakina Zata Amni
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Reland] Use a separate utility process for system proxy resolution for Mac
Original CL: crrev.com/c/7761426 Revert CL: crrev.com/c/7807640
Reason for revert: The original CL was reverted because the newly added
ChromeMojoProxyResolverMacBrowserTest tests failed on CI-only Mac
browser_tests.
- chromium.mac:Mac12 Tests|browser_tests https://ci.chromium.org/ui/p/chromium/builders/ci/Mac12%20Tests/42497/test-results
- chromium.mac:mac15-x64-rel-tests|browser_tests
https://ci.chromium.org/ui/p/chromium/builders/ci/mac15-x64-rel-tests/17348/test-results
The common failing tests were:
- ChromeMojoProxyResolverMacBrowserTest.ServiceLifecycle
- ChromeMojoProxyResolverMacBrowserTest.DestroyAndCreateService
- ChromeMojoProxyResolverMacBrowserTest.DestroyResolver
The concrete failure mode was a failure while initializing the macOS
seatbelt sandbox for the proxy resolver utility process:
test_launcher.cc(368): Check failed:
seatbelt.server->InitializeSandbox().
DestroyResolver also timed out waiting for the service launch:
chrome_mojo_proxy_resolver_mac_browsertest.cc(41): RunLoop::Run() timed
out.
Root cause:
The proxy resolver seatbelt profile (sandbox/policy/mac/proxy_resolver.sb) requires the SYSTEM_PROXY_NETWORK_ACCESS parameter, but SetupSandboxParameters() did not supply it for the kProxyResolver sandbox type. With the parameter missing, seatbelt could not compile the profile and failed with "internal-strcmp: argument 1 must be: string", which causedsandbox initialization to crash.
Fix (included in this reland at Patchset 6):
- content/browser/sandbox_parameters_mac.mm:Handle kProxyResolver case
and supply SYSTEM_PROXY_NETWORK_ACCESS. It is set to false for now;
PAC/WPAD network access stays disabled until that support is added
(crbug.com/442313607).
- content/browser/service_host/utility_sandbox_delegate.cc: add
kProxyResolver to the supported sandbox types on Mac and Windows.
Regression coverage:
Added SandboxMacTest.ProxyResolverInitializesSandbox in
content/browser/sandbox_mac_unittest.mm, which runs the kProxyResolver
policy through SetupSandboxParameters() and initializes the seatbelt
sandbox, the exact path that previously crashed. Verified locally on
macOS: the test fails with the original "internal-strcmp" error without
the fix and passes with it.
This reland is therefore self-contained. It carries the fix for the root
cause rather than relying on a fix landed elsewhere.
Include-Ci-Only-Tests: chromium.mac:Mac12 Tests|browser_tests
Include-Ci-Only-Tests: chromium.mac:mac15-x64-rel-tests|browser_tests
- M chrome/browser/net/BUILD.gn
- A chrome/browser/net/chrome_mojo_proxy_resolver_mac.cc
- A chrome/browser/net/chrome_mojo_proxy_resolver_mac.h
- A chrome/browser/net/chrome_mojo_proxy_resolver_mac_browsertest.cc
- M chrome/browser/net/system_network_context_manager.cc
- M chrome/test/BUILD.gn
- M content/browser/sandbox_mac_unittest.mm
- M content/browser/sandbox_parameters_mac.mm
- M content/browser/service_host/utility_sandbox_delegate.cc
- M services/network/BUILD.gn
- A services/network/mac_system_proxy_resolver_mojo.cc
- A services/network/mac_system_proxy_resolver_mojo.h
- A services/network/mac_system_proxy_resolver_mojo_unittest.cc
- M services/network/network_context.cc
- M services/network/public/mojom/network_context.mojom
- M services/network/url_request_context_builder_mojo.cc
- M services/network/url_request_context_builder_mojo.h
Code-Review: +1 by Avi Drissman, +1 by Kenichi Ishibashi, +1 by Hayato Ito, +1 by Alex Gough
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |