Fix MonitorChild thread violation in utility process #8620

darkdh · 2021-04-23T03:43:46Z

MonitorChild is no longer a member function but still running on child monitor thread, and it will run callback in the original thread using BindPostTask.
We cannot bind crash_handler_callback_ directly in BindPostTask because we need to access some members in OnChildCrash and the callback must be called before it is destructed otherwise we will hit this DCHECK

  static void OnIsConnectedComplete(bool connected) {
    DCHECK(!connected)
        << "TorLauncher::LaunchCallback was destroyed without "
        << "first either being run or its corresponding binding being closed. "
        << "It is an error to drop response callbacks which still correspond "
        << "to an open interface pipe.";
  }

Also introduce ChildProcessMonitor to deduplicate the external child lifetime monitoring logic

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

goodov · 2021-04-23T10:36:15Z

components/services/ipfs/ipfs_service_impl.h

-  DISALLOW_COPY_AND_ASSIGN(IpfsServiceImpl);
+  base::WeakPtrFactory<IpfsServiceImpl> weak_ptr_factory_{this};
+
+  IpfsServiceImpl(const IpfsServiceImpl&) = delete;


methods/constructors should go before members.

fixed in 7757acb

goodov · 2021-04-23T10:38:16Z

components/services/ipfs/ipfs_service_impl.cc

+        } else if (WIFEXITED(status)) {
+          VLOG(0) << "ipfs exit (" << WEXITSTATUS(status) << ")";
+        }
+        if (callback) {


maybe just DCHECK(callback); at start and remove ifs? it's unlikely someone would use it without a callback.

fixed in 7757acb

goodov · 2021-04-23T10:52:09Z

components/services/ipfs/ipfs_service_impl.cc

@@ -108,22 +109,62 @@ bool LaunchProcessAndExit(const base::FilePath& path,
  return true;
 }

+void MonitorChild(base::ProcessHandle p_handle,


can we put this machinery in a dedicated .h/.cc and fix code duplication? ipfs/tor impls clearly cannot be used simultaneously because of global sigaction calls.
it would be nice to have a single entry point for the Setup/TearDownPipeHack, MonitorChild and DCHECK if it's used in the same process for something else.

I will fix the duplication by wrapping a ChildMonitor class and also global sigaction calls don't matter because IpfsServiceImpl and TorLauncherImpl are in different utility processes.

fixed in 7757acb

components/services/ipfs/ipfs_service_impl.cc

components/external_child_process/child_monitor.cc

kkuehlz · 2021-04-26T23:32:16Z

components/external_child_process/child_monitor.cc

+  memset(&action, 0, sizeof(action));
+  action.sa_handler = SIG_DFL;
+  sigaction(SIGCHLD, &action, NULL);
+  for (size_t i = 0; i < 2; ++i)


since there will only ever be two pipe elements a loop doesn't add much.

fixed in fcf4957f689f39e8f09a0c9aa56494935fcbfd7d

goodov · 2021-04-27T07:25:11Z

components/external_child_process/BUILD.gn

@@ -0,0 +1,13 @@
+source_set("child_monitor") {


components/child_process_monitor/BUILD.gn
source_set("child_process_monitor") { ... }?

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T07:25:33Z

components/external_child_process/BUILD.gn

@@ -0,0 +1,13 @@
+source_set("child_monitor") {
+  visibility = [
+    "//brave/components/services/ipfs",


maybe add a comment why we restrict visibility?

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T07:37:21Z

components/external_child_process/child_monitor.cc

+    NOTREACHED();
+  }
+}
+ChildMonitor::~ChildMonitor() {


an empty line is missing

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T07:57:33Z

components/external_child_process/child_monitor.cc

+                  << ")";
+        }
+        std::move(callback).Run(pid);
+      }


break; / return; to be safe?

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T07:59:01Z

components/external_child_process/child_monitor.cc

+}  // namespace
+
+ChildMonitor::ChildMonitor()
+    : child_monitor_thread_(new base::Thread("child_monitor_thread")) {


std::make_unique

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T08:01:45Z

components/external_child_process/child_monitor.cc

+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  child_process_ = std::move(child);
+
+  DCHECK(child_monitor_thread_.get());


just DCHECK(child_monitor_thread_);

goodov · 2021-04-27T08:12:52Z

components/external_child_process/child_monitor.cc

+#include "base/process/kill.h"
+#include "base/single_thread_task_runner.h"
+#include "base/task/post_task.h"
+#include "base/threading/thread_task_runner_handle.h"


sequenced_task_runner_handle.h

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T08:13:52Z

components/external_child_process/child_monitor.cc

+}
+
+#if defined(OS_MAC)
+static void SetupFD(int fd) {


static void SIGCHLDHandler static void SetupFD static void SetupPipeHack static void TearDownPipeHack

static is not necessary. we're already in anonymous namespace.

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T08:16:47Z

components/services/ipfs/ipfs_service_impl.cc

-  // processes created above.
-  SetupPipeHack();
-#endif
+  child_monitor_.reset(new brave::ChildMonitor());


std::make_unique

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

goodov · 2021-04-27T08:44:04Z

components/external_child_process/child_monitor.cc

+#if defined(OS_MAC)
+    // TODO(https://crbug.com/806451): The Mac implementation currently blocks
+    // the calling thread for up to two seconds.
+    base::PostTask(


base::ThreadPool::PostTask

fixed in 76857501ecbfb2f31ff1023ec197de6d90d62618

kkuehlz · 2021-04-29T06:37:02Z

components/child_process_monitor/child_process_monitor.cc

+  if (fcntl(pipehack[1], F_SETFL, flags) == -1)
+    VLOG(0) << "set flags errno:" << errno;
+
+  struct sigaction action;


generally it's a good idea to set SA_RESTART in your sigaction flags, especially since your code is doing a lot of syscalls.

fixed in 2ce89fab2bc58cb8ab04ef80cb0f2efaaa56cbbe

kkuehlz

LGTM

darkdh · 2021-04-30T23:39:50Z

11:26:46  1 test crashed:
11:26:46      RewardsContributionBrowserTest.AutoContributionMultiplePublishersUphold (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:214)

not related to this PR
https://ci.brave.com/job/pr-brave-browser-utility-monitor-child-linux/16/execution/node/221/log/

darkdh self-assigned this Apr 23, 2021

darkdh requested review from bbondy, yrliou, goodov and iefremov April 23, 2021 03:43

darkdh force-pushed the utility-monitor-child branch 2 times, most recently from 1499f09 to 9895a95 Compare April 23, 2021 05:38

goodov reviewed Apr 23, 2021

View reviewed changes

kkuehlz reviewed Apr 23, 2021

View reviewed changes

components/services/ipfs/ipfs_service_impl.cc Outdated Show resolved Hide resolved

darkdh force-pushed the utility-monitor-child branch 3 times, most recently from 7757acb to d74ac50 Compare April 26, 2021 23:16

kkuehlz reviewed Apr 26, 2021

View reviewed changes

darkdh force-pushed the utility-monitor-child branch from d74ac50 to fcf4957 Compare April 27, 2021 05:32

goodov reviewed Apr 27, 2021

View reviewed changes

darkdh force-pushed the utility-monitor-child branch 5 times, most recently from cff041b to 1db72ee Compare April 28, 2021 19:15

darkdh requested review from goodov and kkuehlz April 28, 2021 23:40

darkdh force-pushed the utility-monitor-child branch from 1db72ee to 4a9daf2 Compare April 29, 2021 01:20

kkuehlz reviewed Apr 29, 2021

View reviewed changes

goodov approved these changes Apr 29, 2021

View reviewed changes

darkdh force-pushed the utility-monitor-child branch from 4a9daf2 to 1efc1c6 Compare April 29, 2021 21:16

darkdh requested a review from kkuehlz April 29, 2021 21:16

kkuehlz approved these changes Apr 29, 2021

View reviewed changes

darkdh added 2 commits April 30, 2021 08:44

Fix MonitorChild thread violation in utility process

7245f31

Introduce ChildMonitor to monitor the lifetime of external child process

ffd5e58

darkdh added 3 commits April 30, 2021 08:44

Simplify pipehack

61bb199

ChildMonitor unittest

c404698

Rename ChildMonitor to ChildProcessMonitor and address review feedback

6f3389b

darkdh force-pushed the utility-monitor-child branch from 1efc1c6 to 6f3389b Compare April 30, 2021 15:45

darkdh merged commit eb4d020 into master Apr 30, 2021

darkdh deleted the utility-monitor-child branch April 30, 2021 23:39

darkdh added this to the 1.26.x - Nightly milestone Apr 30, 2021

kylehickinson pushed a commit that referenced this pull request Jan 4, 2024

Fix #8619: Send correct ad notification clicked event (#8620)

e084089

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MonitorChild thread violation in utility process #8620

Fix MonitorChild thread violation in utility process #8620

darkdh commented Apr 23, 2021 •

edited

Loading

goodov Apr 23, 2021

darkdh Apr 26, 2021

goodov Apr 23, 2021

darkdh Apr 26, 2021

goodov Apr 23, 2021

darkdh Apr 26, 2021

darkdh Apr 26, 2021

kkuehlz Apr 26, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

goodov Apr 27, 2021

darkdh Apr 27, 2021

kkuehlz Apr 29, 2021

darkdh Apr 29, 2021

kkuehlz left a comment

darkdh commented Apr 30, 2021

Fix MonitorChild thread violation in utility process #8620

Fix MonitorChild thread violation in utility process #8620

Conversation

darkdh commented Apr 23, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkuehlz left a comment

Choose a reason for hiding this comment

darkdh commented Apr 30, 2021

darkdh commented Apr 23, 2021 •

edited

Loading