1607530 - Crash in [@ mozilla::PreallocatedProcessManager::AddBlocker]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-a6870e9f-ee46-4bd0-817c-aa4810200107.

Seen while looking at nightly crash stats: https://bit.ly/2N2yMIz. So far macOS crashes only.

Possible regression range based on build id: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86aa64c6bd540f6e93e8bde71754a03cb343f5b7&tochange=e6427fac5ee8d1d87fb78e917781e85dda119a81

If I am correct in the component, there are a few bugs that landed in that changeset.

Top 10 frames of crashing thread:

0 XUL mozilla::PreallocatedProcessManager::AddBlocker dom/ipc/PreallocatedProcessManager.cpp:319
1 XUL mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>, mozilla::ipc::LaunchError, false>::ThenValue<mozilla::dom::ContentParent::GetNewOrUsedBrowserProcessAsync xpcom/threads/MozPromise.h:726
2 XUL mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>, mozilla::ipc::LaunchError, false>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:402
3 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1241
4 XUL <name omitted> xpcom/threads/nsThreadUtils.cpp:486
5 XUL nsThread::Shutdown xpcom/threads/nsThread.cpp:913
6 XUL nsThreadPool::Shutdown xpcom/threads/nsThreadPool.cpp:397
7 XUL mozilla::detail::RunnableMethodImpl<nsCOMPtr<nsIThreadPool>, nsresult  xpcom/threads/nsThreadUtils.h:1217
8 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1241
9 XUL NS_ProcessPendingEvents xpcom/threads/nsThreadUtils.cpp:434

Comment 1

[Andrew McCreight [:mccr8]]

Yoric, maybe this is a regression from some of your work on improving process launching? Thanks.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Very likely, yes. Preparing a patch.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1607530 - Fixing lifetime issues in promise closures;r?nika

Comment 4

[Marco Zehe (:MarcoZ)]

Are you planning to land this soon'ish? I just ran into this very crash myself.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0ab7937aa405
Fixing lifetime issues in promise closures;r=nika

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0ab7937aa405

Comment 7

[Jens Stutte [:jstutte]]

Hi Cristian, Luca suggests here to uplift this bug to 73 - agree?

Comment 8

[Cristian Brindusan [:cbrindusan]]

I think maybe you meant :jcristau or :RyanVm :)

Comment 9

[Jens Stutte [:jstutte]]

Oh, sorry. Yes, thanks.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

We haven't seen any crash reports with this signature from Beta73. Do we think this bug is likely to bite us when 73 goes to release?

Comment 11

[Jens Stutte [:jstutte]]

I was just the messenger bringing in Luca's comment - Luca?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

I think that it would be reasonable to ask David's opinions on this, and so I'm redirecting the needinfo to him.

(from my point of view, given the kind of issue fixed and where it is applied, ContentParent::GetNewOrUsedBrowserProcessAsync, it seems like it may start to bite us at some point and so I think it would be reasonable to fix it before it gets to release).

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Sorry for the slow response, I'm sick. There is a chance that the bug might bite us in 73, so I'd be in favor of uplifting.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

You can nominate it if you want, we'll just need a clear assessment of risk vs. reward since we're just over a week away from 73 going to RC.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)

You can nominate it if you want, we'll just need a clear assessment of risk vs. reward since we're just over a week away from 73 going to RC.

Hum. I don't think I ever nominated something for uplift. How do I do that?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

On the attachment details, set the approval-mozilla-beta dropdown to '?' and answer the questions that get asked. The more detailed the replies, the better.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9119346 [details]
Bug 1607530 - Fixing lifetime issues in promise closures;r?nika

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, possible security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a trivial fix that resolves dangling pointer access.
• String changes made/needed:

Comment 18

[Julien Cristau [:jcristau]]

Hey David, per our conversation on matrix last week we may be OK on 73; did you get a chance to double check?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119346 [details]
Bug 1607530 - Fixing lifetime issues in promise closures;r?nika

Let's let this ride with 74.