1763625 - Intermittent SUMMARY: ThreadSanitizer: data race /builds/worker/checkouts/gecko/ipc/chromium/src/base/process_util_posix.cc:52:11 in OpenProcessHandle

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: alissy [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=373605066&repo=try
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Pr8csIIpSu6wLQQPTe7F9Q/runs/0/artifacts/public/logs/live_backing.log

---

Comment 1

[Andrew McCreight [:mccr8]]

This looks familiar.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The spike in recent failures seems to be from bug 1787718 based on the stack.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hm, ContentParent::Pid()/::GetOsPid() are racy. The stack trace for example in
https://treeherder.mozilla.org/logviewer?job_id=389272588&repo=autoland&lineNumber=2404 shows a code path where any JS could access GetOsPid() which then just calls Pid() and apparently that isn't ok.

jld, could we somehow fix ContentParent::Pid()?
We can backout bug 1787718 for now, but that isn't really the cause, it just makes us call Pid() more often.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that there is nothing that I can do here, I would suggest that we backout the patch. Aryx, could you please take care of it?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The patch on bug 1787718 got backed out:
https://hg.mozilla.org/integration/autoland/rev/972d0006abb1b5160fa87e1754f2f1d23975c3d9

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #3)

Hm, ContentParent::Pid()/::GetOsPid() are racy. The stack trace for example in
https://treeherder.mozilla.org/logviewer?job_id=389272588&repo=autoland&lineNumber=2404 shows a code path where any JS could access GetOsPid() which then just calls Pid() and apparently that isn't ok.

jld, could we somehow fix ContentParent::Pid()?
We can backout bug 1787718 for now, but that isn't really the cause, it just makes us call Pid() more often.

Jed, do you have some feedback for us?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

My idea here is to protect mChildProcessHandle with a lock, and then get the pid under that lock (so, maybe add GeckoChildProcessHost::GetChildProcessId). That will avoid racing on the handle or doing a use-after-close (on Windows; on Unix a “handle” is a pid), but the pid might not refer to the process after the lock is released (if the process exits). If the pid is used only for logging and not to interact with the process, that should be an acceptable risk.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interestingly we can see those assertions / crashes now even on mozilla-central since November 29th and it's always the test browser_test_powerMetrics.js which is causing it.

https://treeherder.mozilla.org/logviewer?job_id=398502658&repo=mozilla-central&lineNumber=4234

Florian made some changes here recently on bug 1802361, so probably those triggered this particular crash.

Comment 9

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)

Interestingly we can see those assertions / crashes now even on mozilla-central since November 29th and it's always the test browser_test_powerMetrics.js which is causing it.

https://treeherder.mozilla.org/logviewer?job_id=398502658&repo=mozilla-central&lineNumber=4234

If I understand these stacks, they just mean the "IPC I/O Parent" thread called GeckoChildProcessHost::AsyncLaunch after the main thread called ChromeUtils::RequestProcInfo (which called GeckoChildProcessHost::GetAll).

I don't think these calls should be a problem, so it's probably at the GeckoChildProcessHost level that something needs fixing. And that seems aligned with what Jed said in comment 7.

Florian made some changes here recently on bug 1802361, so probably those triggered this particular crash.

The only explanation I can see is that maybe the test now takes longer to complete.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

browser_test_powerMetrics.js is causing this crash pretty frequently on Linux. Florian, would it be something you could at least investigate?

Comment 11

[Florian Quèze [:florian]]

I don't think I can do more than what I wrote in comment 9.
If we can't fix this and the failure rate becomes a problem, we might need to just disable the test on tsan.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧ from comment #7)

My idea here is to protect mChildProcessHandle with a lock, and then get the pid under that lock

Is this something that you could easily do, or was there a problem with this idea?

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Jed said he had a partial patch for this issue, so let's take it.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1763625 - Refactor how we access child process handles.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/dc92467cea22653feff3eb52e706d664b84f3394

Backed out for causing memory-related crashes and failures

backout: https://hg.mozilla.org/integration/autoland/rev/73dfb11f4338225d2e930e583ca37a3ad01f940c
push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=dc92467cea22653feff3eb52e706d664b84f3394
failure log: https://treeherder.mozilla.org/logviewer?job_id=407189999&repo=autoland&lineNumber=4099

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Silly mistake on my part: the old version of ContentParent::Pid had this:

  if (!mSubprocess || !mSubprocess->GetChildProcessHandle()) {
    return -1;
  }

And I accidentally dropped the !mSubprocess check, so it dereferences a null pointer in that case. Should be easy enough to fix.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Btw this got re-landed as: https://hg.mozilla.org/integration/autoland/rev/72094220e38c3726f8a24a6306429c17882399c7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72094220e38c

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jld, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It's already ridden the train to beta at this point, but for context: The severity is relatively minor and the patch isn't trivial, so I wasn't in favor of uplift.