Closed Bug 1607530 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- unaffected
firefox73 --- wontfix
firefox74 --- fixed

People

(Reporter: marcia, Assigned: Yoric)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

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

Flags: needinfo?(dteller)

Very likely, yes. Preparing a patch.

Flags: needinfo?(dteller)
Assignee: nobody → dteller
Status: NEW → ASSIGNED

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

Flags: needinfo?(dteller)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0ab7937aa405
Fixing lifetime issues in promise closures;r=nika
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: needinfo?(dteller)

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

Flags: needinfo?(cbrindusan)

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

Flags: needinfo?(cbrindusan) → needinfo?(ryanvm)

Oh, sorry. Yes, thanks.

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?

Flags: needinfo?(ryanvm) → needinfo?(jstutte)

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

Flags: needinfo?(jstutte) → needinfo?(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).

Flags: needinfo?(lgreco) → needinfo?(dteller)

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.

Flags: needinfo?(dteller) → needinfo?(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.

Flags: needinfo?(ryanvm) → needinfo?(dteller)

(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?

Flags: needinfo?(dteller) → needinfo?(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.

Flags: needinfo?(ryanvm)

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:
Attachment #9119346 - Flags: approval-mozilla-beta?

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

Flags: needinfo?(dteller)

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

Let's let this ride with 74.

Flags: needinfo?(dteller)
Attachment #9119346 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.