1703332 - Crash in [@ nsNavHistoryResult::HandlePlacesEvent]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

This signature went from 1-2 crashes per release to 10-30 crashes per beta version (gecko version is the first frequently affected one). The code in stack frame #0 is from bug 897954.

Daisuke, please investigate.

Crash report: https://crash-stats.mozilla.org/report/index/d3e4055e-e8ff-4a32-966b-7c2f00210406

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsNavHistoryResult::HandlePlacesEvent toolkit/components/places/nsNavHistoryResult.cpp:4222
1 xul.dll static mozilla::dom::PlacesObservers::NotifyListeners dom/base/PlacesObservers.cpp:296
2 xul.dll mozilla::dom::PlacesObservers_Binding::notifyListeners dom/bindings/PlacesObserversBinding.cpp:404
3 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:520
4 xul.dll Interpret js/src/vm/Interpreter.cpp:3244
5 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:552
6 xul.dll js::Call js/src/vm/Interpreter.cpp:597
7 xul.dll js::CallSelfHostedFunction js/src/vm/SelfHosting.cpp:1642
8 xul.dll js::jit::InterpretResume js/src/jit/VMFunctions.cpp:1306
9  @0x1e9aaf24adf 

Comment 1

[Daisuke Akatsuka (:daisuke)]

Thank you for letting me know, Sebastian.
I will take a look at this issue.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1703332: Remove purge_caches listener before unlinking mRootNode.r?mak!

Until now, we have removed the listener for purge_caches on the destructor.
https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/toolkit/components/places/nsNavHistoryResult.cpp#3482-3484
But, when closing, since NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN will be called before calling the destructor, even if mRootNode is unlinked, might capture the purge_caches event.
https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/toolkit/components/places/nsNavHistoryResult.cpp#3411
And in the case, it will crash due to touch the null object.
In this change, remove the listener before unlinking the mRootNode explicitly.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acd4f182c089
Remove purge_caches listener before unlinking mRootNode.r=mak

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/acd4f182c089

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

please ask for Beta uplift when you have a chance.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Thanks, Marco. I will do it.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 9213977 [details]
Bug 1703332: Remove purge_caches listener before unlinking mRootNode.r?mak!

Beta/Release Uplift Approval Request

• User impact if declined: Depending on the timing, it may crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): We changed just the timing of removing the observer.
• String changes made/needed: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9213977 [details]
Bug 1703332: Remove purge_caches listener before unlinking mRootNode.r?mak!

Fingers crossed that it helps with the spike. Approved for 88.0rc1.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/abb886cee217