1721487 - Crash in [@ mozilla::telemetry::Timers::StartUserInteraction]

Status: RESOLVED WORKSFORME

(Toolkit :: Telemetry, defect, P4)

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/7e98d409-eae9-4744-8d95-59fc80210720

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!listElem->isInList())

Top 9 frames of crashing thread:

0 xul.dll mozilla::telemetry::Timers::StartUserInteraction toolkit/components/telemetry/core/Stopwatch.cpp:455
1 xul.dll mozilla::dom::UserInteraction_Binding::start dom/bindings/UserInteractionBinding.cpp:72
2 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:511
3 xul.dll Interpret js/src/vm/Interpreter.cpp:3226
4 xul.dll js::Call js/src/vm/Interpreter.cpp:588
5 xul.dll nsXPCWrappedJS::CallMethod js/xpconnect/src/XPCWrappedJSClass.cpp:971
6 xul.dll PrepareAndDispatch xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:79
7 xul.dll SharedStub xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:98
8 xul.dll NS_InvokeByIndex 

Another LinkedList issue detected by Nika's patch in bug 1717778 (so likely not a regression per se).

Comment 1

[Andrew McCreight [:mccr8]]

It looks like maybe a timer might not be started but still is already in a linked list?

Mike, any idea what might be going wrong here? Thanks.

Comment 2

[Daniel Veditz [:dveditz]]

Calling this sec-moderate because this is in response to user interaction and not directly influenced by web content. Also, branches with this assert aren't vulnerable because of the assert so the vulnerable branches are the older ones, but we don't know how far back. Maybe we just let the asserts ride the trains to 91/ESR-91 (fixing the potential vulnerability) and treat fixing this crash as a stability issue if it starts happening a lot.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hm. This is pretty mysterious. It's not immediately obvious to me why the timer reference would already be in the list.

I think we can be overly cautious here and move Stopwatch.cpp line 444 to 436, to always remove the timer from the list before inserting it.

https://searchfox.org/mozilla-central/rev/538569c63ffb17d590c51df66f9fff790dd7431e/toolkit/components/telemetry/core/Stopwatch.cpp#436,444

What do you think, dthayer?

Comment 4

[Alex Thayer [:alexical] (she/her)]

Mmm. I've been staring at this for a while now (probably too long given the low frequency of this bug). It's really ringing my "we should understand this before we try to just defensively pave over it" alarm bells. That being said, I can't make any sense of how we could get in this situation.

The only thing I could think of that could get us here is if somehow TimeStamp::Now() is giving us a null timestamp. Looking into it on Windows it looks kind of possible? But very unlikely.

I don't know. I guess that's about as far into it as I can look, and I suppose we could at least experiment with just always removing the timer from the list if isInList is true. If that does or doesn't work we at least have more information.

Comment 5

[Andrew McCreight [:mccr8]]

Bug 1717778 is on all branches now so this shouldn't be a security issue any more.

Comment 6

[Marco Castelluccio [:marco]]

:chutten, given that this is not a security issue anymore, could you reassess the severity?

Comment 7

[Chris H-C :chutten]

Certainly now seems to be happily in the "very little impact" camp. Gonna reraise :dothayer on the ni? phone to ask if we should "just" resolve this as a result of bug 1717778

Comment 8

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

Closing because no crashes reported for 12 weeks.