Closed Bug 1339010 Opened 3 years ago Closed 3 years ago
Assertion failure: is
Valid (Accumulation using invalid id), at Telemetry Histogram .cpp:1334
110.94 KB, text/plain
110.28 KB, text/plain
59 bytes, text/x-review-board-request
1.41 KB, patch
|Details | Diff | Splinter Review|
Bughunter run into this while loading https://9anime.to/watch/eldlive-dub.p1vq/pjjyvq Talked to dexter and seems this might be interesting for you guys. From the logs it seems that just visiting this site caused the assertion but still looking into it. Also it might be related to bug 1338814 somehow
Priority: -- → P2
I can't immediately trigger this on Nightly. Is this with e10s on or off? Which Firefox version and platform? For STR, is allowing Flash required? Quickly checking over SSLServerCertVerification.cpp, there are quite a few histogram accumulations, most of them directly using Telemetry enum ids. It would be good to at least track it to the specific line.
(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > I can't immediately trigger this on Nightly. > > Is this with e10s on or off? e10s is on > Which Firefox version and platform? aurora debug: Windows NT 10.0 x86 32/32 Windows NT 6.1 x86 32/32 3 beta debug: Windows NT 6.1 x86 32/32 Windows NT 10.0 x86 32/32 3 nightly debug: Windows NT 10.0 x86 32/32 > For STR, is allowing Flash required? i don't think we have flash installed on the test workers anymore (bc do you know?) > > Quickly checking over SSLServerCertVerification.cpp, there are quite a few > histogram accumulations, most of them directly using Telemetry enum ids. > It would be good to at least track it to the specific line.
Which channel and build is the stack here from?
this is from a windows 10 trunk build from mozilla-central
channel is i guess nightly then (since this is also the default). We take the builds from m-c (like latest m-c build via taskcluster)
This is more helpful, thanks. This points us specifically here: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/security/manager/ssl/SSLServerCertVerification.cpp#1369 ... which is one the places that uses a variable for the histogram id. Cykesiopka, i see you recently touched these files, do you have an idea on why `pinningTelemetryInfo.certPinningResultHistogram` could contain invalid values? mgoodwin also might have context as i see he reworked the key pinning telemetry at some point.
Component: Telemetry → Security: PSM
Priority: P2 → --
Product: Toolkit → Core
Whiteboard: [measurement:client] → [measurement:client:tracking]
Well, it looks like some of that code might be using uninitialized values.
Assignee: nobody → dkeeler
Priority: -- → P1
Whiteboard: [measurement:client:tracking] → [measurement:client:tracking][psm-assigned]
Bug 1339365 shows that 51+ is affected. If this histogram is important for monitoring, you should consider uplifting this patch to 52+.
Comment on attachment 8836949 [details] bug 1339010 - ensure pinning and CT telemetry info has been initialized https://reviewboard.mozilla.org/r/112254/#review113804 This looks like it should resolve the uninitialized value errors. LGTM.
Attachment #8836949 - Flags: review?(jjones) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/fb7b874ca753 ensure pinning and CT telemetry info has been initialized r=jcj
David : per comment #9 could you request uplifting ? Thanks!
Approval Request Comment [Feature/Bug causing the regression]: long-standing issue [User impact if declined]: potential assertion failures, bogus telemetry data for us [Is this code covered by automated tests?]: this area of code, yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no (I'm writing a test that will cover exactly this case, but it first requires fixing bug 1339923) [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: initializing memory is almost always more correct than not [String changes made/needed]: none
Comment on attachment 8838242 [details] [diff] [review] patch for aurora/beta Fix an assertion failure. Aurora53+.
Attachment #8838242 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8838242 [details] [diff] [review] patch for aurora/beta tls telemetry fix, beta52+
Attachment #8838242 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.