Closed
Bug 1339365
Opened 6 years ago
Closed 6 years ago
Crash in `anonymous namespace''::internal_GetHistogramByEnumId
Categories
(Toolkit :: Telemetry, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: philipp, Assigned: gfritzsche)
References
Details
(Keywords: crash, Whiteboard: [measurement:client])
Crash Data
Attachments
(1 file)
7.28 KB,
patch
|
chutten
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-57ab43b4-f014-4bfb-952e-325df2170212. ============================================================= Crashing Thread (65) Frame Module Signature Source 0 xul.dll `anonymous namespace'::internal_GetHistogramByEnumId toolkit/components/telemetry/TelemetryHistogram.cpp:418 1 xul.dll TelemetryHistogram::Accumulate(mozilla::Telemetry::ID, unsigned int) toolkit/components/telemetry/TelemetryHistogram.cpp:1901 2 xul.dll mozilla::psm::`anonymous namespace'::AuthCertificate security/manager/ssl/SSLServerCertVerification.cpp:1288 3 xul.dll mozilla::psm::`anonymous namespace'::SSLServerCertVerificationJob::Run security/manager/ssl/SSLServerCertVerification.cpp:1439 4 xul.dll nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:225 5 xul.dll nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1067 6 xul.dll NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:311 7 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:338 8 xul.dll MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:225 9 xul.dll MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:205 10 xul.dll nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:465 11 nss3.dll _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:397 12 nss3.dll pr_root nsprpub/pr/src/md/windows/w95thred.c:95 13 ucrtbase.dll thread_start<unsigned int (__stdcall*)(void*)> 14 kernel32.dll BaseThreadInitThunk 15 ntdll.dll __RtlUserThreadStart 16 ntdll.dll _RtlUserThreadStart this crash was spiking up on 2017-02-12 for a day on 32bit versions of firefox 51.0.1 on windows. many of the user comments mentioned a site called 9anime.to in relation to this kind of crash. https://mozilla.github.io/stab-crashes/supergraph.html?s=`anonymous+namespace%27%27%3A%3Ainternal_GetHistogramByEnumId
Assignee | ||
Comment 1•6 years ago
|
||
The triggering bug here is probably bug 1339010, where in SSLServerCertVerification.cpp uninitialized variables are being used for histogram ids. However, we should probably catch this path to protect from errors in other components. The crash occurs here, so a simple (and cheap) bounds check on the histogram id should be sufficient: https://hg.mozilla.org/releases/mozilla-release/annotate/327e081221b0/toolkit/components/telemetry/TelemetryHistogram.cpp#l418
Assignee | ||
Updated•6 years ago
|
Priority: -- → P1
Whiteboard: [measurement:client] [qa+]
Assignee | ||
Updated•6 years ago
|
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gfritzsche
Assignee | ||
Comment 2•6 years ago
|
||
[Tracking Requested - why for this release]: Crash bug. Triggered from security/manager/ssl/ code, but best protected against from Telemetry code.
tracking-firefox52:
--- → ?
Assignee | ||
Comment 3•6 years ago
|
||
This moves the histogram id checks consistently to the public Histogram API. Chris, can you review? Nathan, do you have any concerns with this?
Attachment #8837268 -
Flags: review?(chutten)
Attachment #8837268 -
Flags: feedback?(nfroyd)
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27cfca7e8a901cc9dd6ddc8365dc492150d7676e
Comment 5•6 years ago
|
||
Comment on attachment 8837268 [details] [diff] [review] Move Histogram ID validity checks to outer API Review of attachment 8837268 [details] [diff] [review]: ----------------------------------------------------------------- I like the concept and the execution. The ipc refactor hasn't landed, has it? Because, if so, this won't apply.
Attachment #8837268 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 8837268 [details] [diff] [review] Move Histogram ID validity checks to outer API Actually this seems clear & simple enough, cancelling the additional feedback.
Attachment #8837268 -
Flags: feedback?(nfroyd)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•6 years ago
|
Whiteboard: [measurement:client] [qa+] → [measurement:client] [measurement:client:uplift] [qa+]
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Chris H-C :chutten from comment #5) > The ipc refactor hasn't landed, has > it? Because, if so, this won't apply. Right - the refactor will land with bug 1313326, which will land later.
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e9eea586ed Move Histogram ID validity checks to outer API. r=chutten
Keywords: checkin-needed
tracking-firefox53:
--- → +
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d8e9eea586ed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8837268 [details] [diff] [review] Move Histogram ID validity checks to outer API Approval Request Comment [Feature/Bug causing the regression]: Telemetry, recently triggered by security code bug. [User impact if declined]: Risk of crash from out-of-bounds data access. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Currently baking. [Needs manual test from QE? If yes, steps to reproduce]: Per bug 1339010, comment 0, which should be confirmation steps for beta/aurora if bug 1339010 is not fixed there yet. [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: Low-risk. [Why is the change risky/not risky?]: This is a limited moving of code - an identifier validity check - to an outer API and skipping code if the check fails. [String changes made/needed]: None.
Attachment #8837268 -
Flags: approval-mozilla-beta?
Attachment #8837268 -
Flags: approval-mozilla-aurora?
Updated•6 years ago
|
Comment 11•6 years ago
|
||
Comment on attachment 8837268 [details] [diff] [review] Move Histogram ID validity checks to outer API Fix a crash. Aurora53+.
Attachment #8837268 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•6 years ago
|
||
Comment on attachment 8837268 [details] [diff] [review] Move Histogram ID validity checks to outer API fix crash in telemetry code, beta52+
Attachment #8837268 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/50b11ab79661
Comment 14•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b9a1b65ec610
Comment 15•6 years ago
|
||
Flagging this for verification, instructions in Comment 10.
Flags: qe-verify+
Assignee | ||
Updated•6 years ago
|
Whiteboard: [measurement:client] [measurement:client:uplift] [qa+] → [measurement:client] [qa+]
Comment 16•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/b9a1b65ec610
status-firefox-esr52:
--- → fixed
Comment 17•5 years ago
|
||
Was unable to reproduce the initial crash using steps from bug 1339010, comment 0 using old Nightly debug build from 2017-02-12, normal Nightly from the same date and Fx 51.0.1 on Windows 7 and 10 both x86. If we look at Socorro we can't see any crashes in the last few days in 52. https://crash-stats.mozilla.com/signature/?product=Firefox&signature=%60anonymous%20namespace%27%27%3A%3Ainternal_GetHistogramByEnumId&date=%3E%3D2017-02-14T14%3A05%3A00.000Z&date=%3C2017-02-21T14%3A05%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports Will this be enough to call this Fixed on 52?
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 18•5 years ago
|
||
Thanks, i think crash-stats showing this as gone on 52 is sufficient.
Flags: needinfo?(gfritzsche)
Comment 19•5 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #18) > Thanks, i think crash-stats showing this as gone on 52 is sufficient. Thank you! Marking accordingly.
Flags: qe-verify+
Comment 20•5 years ago
|
||
I see a few crashes newer than 2016-02-16: https://crash-stats.mozilla.com/signature/?build_id=%3E20170216094131&signature=%60anonymous%20namespace%27%27%3A%3Ainternal_GetHistogramByEnumId&date=%3E%3D2017-02-22T14%3A37%3A10.000Z&date=%3C2017-03-08T14%3A37%3A10.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports Thoughts?
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 21•5 years ago
|
||
I only see like 3 for each channel & version in the last 14 days. That seems like noise. https://crash-stats.mozilla.com/signature/?build_id=%3E20170216094131&signature=%60anonymous%20namespace%27%27%3A%3Ainternal_GetHistogramByEnumId&date=%3E%3D2017-03-09T11%3A18%3A07.000Z&date=%3C2017-03-23T11%3A18%3A07.000Z
Flags: needinfo?(gfritzsche)
Comment 22•5 years ago
|
||
Verified fixed based on comment 21.
Status: RESOLVED → VERIFIED
Whiteboard: [measurement:client] [qa+] → [measurement:client]
You need to log in
before you can comment on or make changes to this bug.
Description
•