1664728 - startup Crash in [@ mozCreateComponent<T>] for nsITelemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Wayne Mery (:wsmwk)]

Crash report: https://crash-stats.mozilla.org/report/index/5154be17-0869-42bb-b632-47d730200912

Top 10 frames of crashing thread:

0  @0x18 
1 xul.dll mozCreateComponent<nsITelemetry> toolkit/components/telemetry/core/Telemetry.cpp:2252
2 xul.dll mozilla::xpcom::CreateInstanceImpl xpcom/components/StaticComponents.cpp:12489
3 xul.dll mozilla::xpcom::StaticModule::CreateInstance const xpcom/components/StaticComponents.cpp:13379
4 xul.dll nsComponentManagerImpl::GetServiceLocked xpcom/components/nsComponentManager.cpp:1372
5 xul.dll nsComponentManagerImpl::GetServiceByContractID xpcom/components/nsComponentManager.cpp:1559
6 xul.dll nsGetServiceByContractID::operator const xpcom/components/nsComponentManagerUtils.cpp:243
7 xul.dll nsCOMPtr_base::assign_from_gs_contractid xpcom/base/nsCOMPtr.cpp:82
8 xul.dll mozilla::Telemetry::Init toolkit/components/telemetry/core/Telemetry.cpp:2108
9 xul.dll NS_InitXPCOM xpcom/build/XPCOMInit.cpp:478

Comment 1

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/releases/mozilla-esr78/file/tip/toolkit/components/telemetry/core/Telemetry.cpp#l2252
Just out of memory, or something like this?

Comment 2

[Chris H-C :chutten]

Hm, those line numbers for Telemetry.cpp aren't right (it's a long file, but it doesn't have quite 2100 lines (yet)). And there are at least a couple reports from Firefox with the same stack, so it's definitely not Thunderbird-only. (Though none since builds in August)

I don't know what to do with this.

Comment 3

[Magnus Melin [:mkmelin]]

The report is for 78, and there Telemetry.cpp seems to have 2253 lines. I guess it shrunk.

Comment 4

[Chris H-C :chutten]

Oh, that makes more sense, then. Thanks.

Hm. If it's an OOM, why isn't it crashing like an OOM? It wouldn't be EXCEPTION_ACCESS_VIOLATION_EXEC, which points to something weirder than that.

kmag, you know NS_IMPL_COMPONENT_FACTORY. Any ideas? (It's okay if the answer is no)

Comment 5

[Kris Maglione [:kmag]]

I don't think it has anything to do with NS_IMPL_COMPONENT_FACTORY. That just generates a templatized function for the component manager to call, but the crash is happening in the function itself. If I had to guess, I'd say CreateTelemetryInstance is being inlined, and the line numbers in the stack are wrong. Beyond that, it's hard to say what's happening, but if I had to take a stab in the dark, I'd say it's likely because telemetry is being assigned to sTelemetry and the lock released before it's actually addrefed, which means if another thread acquires the lock, addrefs, and then releases the pointer before we get to the NS_ADDREF, it'll have been freed by the time we try to use it.

Just a guess, though. Either way, I think that's a hole that needs to be fixed.

Comment 6

[Chris H-C :chutten]

Attachment: Bug 1664728 - Addref Telemetry instance before dropping the lock r?janerik!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36cde01b37ff
Addref Telemetry instance before dropping the lock r=janerik

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/36cde01b37ff

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/ESR78 approval when you get a chance.

Comment 10

[Chris H-C :chutten]

Comment on attachment 9176140 [details]
Bug 1664728 - Addref Telemetry instance before dropping the lock r?janerik!

Beta/Release Uplift Approval Request

• User impact if declined: An infrequent startup crash may continue.
• 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): This is a very small change moving an addref to be protected by a mutex. It seems highly unlikely that this will have any negative effect, and it might just brighten some users' days.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Startup crashes aren't great, and this has a possibility of stopping some. Plus, it should be a straightforward patch to apply as this code hasn't undergone many changes.
• User impact if declined: Infrequent startup crashes will continue
• Fix Landed on Version: 83 (82 if uplifted)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very small change moving an addref to be protected by a mutex.
• String or UUID changes made by this patch:

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9176140 [details]
Bug 1664728 - Addref Telemetry instance before dropping the lock r?janerik!

approved for 82.0b4 and 78.4esr

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/5abdd17b3c56

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/614b3602848c