Crash in std::_Tree<T>::find

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yoasif, Assigned: bechen)

Tracking

({crash, regression})

56 Branch
mozilla58
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(crash signature)

Attachments

(2 attachments)

This bug was filed from the Socorro interface and is 
report bp-aa04e1e9-4617-4582-991c-a32fa0171013.
=============================================================

Only 17 crashes in the last week, but didn't see it logged in Bugzilla. Affects v57.
Component: General → Audio/Video
Blocks: 1393399
Flags: needinfo?(kaku)
Keywords: regression
https://hg.mozilla.org/releases/mozilla-beta/annotate/b92b69f3503e/dom/media/MediaFormatReader.cpp#l109

This code isn’t thread safe. It could be accessed from multiple threads, erasing an element on one thread, to find in another is bound to create nastiness.
Kaku is on "P"TO. 
Benjamin,
Can you help check it?
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(kaku) → needinfo?(bechen)
Priority: -- → P2
(In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> Kaku is on "P"TO. 
> Benjamin,
> Can you help check it?

Looks like we need a mutex lock to protect it.
Flags: needinfo?(bechen)
Comment on attachment 8918747 [details]
Bug 1408693 - Add a lock to protect the sGPUCrashDataMap.

https://reviewboard.mozilla.org/r/189578/#review194718
Attachment #8918747 - Flags: review?(jwwang) → review+
Assignee: nobody → bechen
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/615798b405f4
Add a lock to protect the sGPUCrashDataMap. r=jwwang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/615798b405f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this worth uplifting to 57?  Crash volume seems low, OTOH the bug sounds possibly nasty.
Flags: needinfo?(bechen)
I believe we need uplift this patch to 57, because the telemetry result is very important for "de-blacklist". Once the crash happened, we can not gather the GPU crash status due to "de-blacklist". That will mislead us to decide the blacklist should be exist or not.

Approval Request Comment
[Feature/Bug causing the regression]: 1393399
[User impact if declined]: Low possibility crash when trying to report telemetry.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No, very hard to reproduce.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: code logic very simple.
[String changes made/needed]: none
Flags: needinfo?(bechen)
Attachment #8919549 - Flags: approval-mozilla-beta?
Comment on attachment 8919549 [details] [diff] [review]
bug1408693.beta.patch

Better telemetry data, low risk, Beta57+
Attachment #8919549 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Jean-Yves, fyi, I took this patch in beta57. If you believe this one is better off staying in 58 train, please let me know. Thanks!
Flags: needinfo?(jyavenard)
I think this one is pretty safe for uplifting...
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.