1824697 - Crash in [@ nsObserverService::EnsureValidCall | nsObserverService::RemoveObserver | LocalesChangedObserver::Unregister]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/dd505b68-cee2-4698-b3be-26bbc0230327

MOZ_CRASH Reason: MOZ_CRASH(Using observer service off the main thread!)

Top 10 frames of crashing thread:

0  xul.dll  nsObserverService::EnsureValidCall const  xpcom/ds/nsObserverService.cpp:171
0  xul.dll  nsObserverService::RemoveObserver  xpcom/ds/nsObserverService.cpp:230
1  xul.dll  LocalesChangedObserver::Unregister  widget/nsBaseWidget.cpp:289
1  xul.dll  nsBaseWidget::FreeLocalesChangedObserver  widget/nsBaseWidget.cpp:381
1  xul.dll  nsBaseWidget::~nsBaseWidget  widget/nsBaseWidget.cpp:400
2  xul.dll  nsWindow::~nsWindow  widget/windows/nsWindow.cpp:782
3  xul.dll  nsBaseWidget::Release  widget/nsBaseWidget.cpp:130
3  xul.dll  mozilla::RefPtrTraits<nsWindow>::Release  mfbt/RefPtr.h:50
3  xul.dll  RefPtr<nsWindow>::ConstRemovingRefPtrTraits<nsWindow>::Release  mfbt/RefPtr.h:381
3  xul.dll  RefPtr<nsWindow>::~RefPtr  mfbt/RefPtr.h:81

It seems we're destroying a window off the main thread and the cascade of operations is trying to unregister an observer which can only be done in the main thread. I'm not 100% sure who's at fault here so I'm putting this in the widget component for now. This crash signature was probably hidden in bug 1276919.

Comment 1

[Gabriele Svelto [:gsvelto]]

Added a signature for older Firefox versions.

Comment 2

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on beta

:handyman, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 3

[Greg Stoll :gstoll]

It looks like UpdateWorkspaceIdTask (which runs on a background thread) is holding on to a RefPtr<nsWindow>, which means if it's the last reference to that nsWindow it will release it on the background thread, leading to this crash. I think making the task just use a WeakPtr instead should address this.

Comment 4

[Greg Stoll :gstoll]

Attachment: Bug 1824697 - do not release nsWindow on a background thread r=#win-reviewers!

We avoid this by only holding on to a WeakPtr of the nsWindow so that
the UpdateWorkspaceIdTask will never hold the last reference to the
nsWindow, so it should never Release() it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a490233c3d0
do not release nsWindow on a background thread r=win-reviewers,handyman

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4a490233c3d0

Comment 7

[Dianna Smith [:diannaS]]

:gstoll did you want to uplift this to 138beta?

Comment 8

[Greg Stoll :gstoll]

Yeah, I think this is worth doing. Thanks!

Comment 9

[Greg Stoll :gstoll]

Attachment: Bug 1824697 - do not release nsWindow on a background thread r=#win-reviewers!

We avoid this by only holding on to a WeakPtr of the nsWindow so that
the UpdateWorkspaceIdTask will never hold the last reference to the
nsWindow, so it should never Release() it.

Original Revision: https://phabricator.services.mozilla.com/D243537

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: users continue to hit a topcrash
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: Just taking a weak reference instead of a strong reference. I think it is more likely that this doesn't fix the crash (since we don't have STR), but it should be safe
• String changes made/needed: no
• Is Android affected?: no

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/799dee327559

Comment 12

[Greg Stoll :gstoll]

Looks like these crashes have indeed gone away in 138 🎉