1710152 - Consider removing WMI code

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, enhancement)

Description

[Toshihito Kikuchi [:toshi]]

The third-party module ping shows lots of third-party AMSI modules are loaded into the browser process through the code to access WMI services. We do this off the main thread, but removing this benefits us from performance and stability perspective.

We use WMI to annotate a crash report with BIOS Manufacturer (bug 921609) and MemoryErrorCorrection (bug 1489297). It seems to be safe to drop BIOS Manufacturer based on this comment.

0 mozglue!patched_LdrLoadDll(wchar_t*, unsigned long*, _UNICODE_STRING*, void**)+0x1d5
1 kernelbase!LoadLibraryExW+0x162
2 Amsi!AmsiComSecureLoadInProcServer+0x29e
3 Amsi!AmsiComCreateProviders<IAntimalwareProvider>+0x15c
4 Amsi!long CAmsiAntimalware::FinalConstruct+0xd0
5 Amsi!static long ATL::CComCreator<class ATL::CComObject<class CAmsiAntimalware> >::CreateInstance(void *,struct _GUID const &,void * *)+0xdf
6 Amsi!virtual long ATL::CComClassFactory::CreateInstance(struct IUnknown *,struct _GUID const &,void * *)+0x58
7 Amsi!AmsiInitialize+0x1af
8 fastprox!void JAmsi::JAmsiInitializeContext+0x4c
9 fastprox!CWbemSvcWrapper::CWbemSvcWrapper(class CLifeControl *,struct IUnknown *)+0x93
10 fastprox!long CSvcProxyBuffer::Init+0x3d
11 fastprox!virtual long CSvcFactoryBuffer::XSvcFactory::CreateProxy(struct IUnknown *,struct _GUID const &,struct IRpcProxyBuffer * *,void * *)+0xc1
12 combase!CStdMarshal::CreateProxy(_GUID const&, _GUID const&, OXIDEntry*, IRpcProxyBuffer**, void**, int*)+0x1e4
13 combase!CStdMarshal::MakeCliIPIDEntry(_GUID const&, tagSTDOBJREF*, OXIDEntry*, tagIPIDEntry**)+0x69
14 combase!CStdMarshal::UnmarshalIPID(_GUID const&, tagSTDOBJREF*, OXIDEntry*, void**)+0x7a
15 combase!CStdMarshal::UnmarshalObjRef(tagOBJREF&, void**)+0x1dd
16 combase!CoUnmarshalInterface(IStream*, _GUID const&, void**)+0x4bc
17 combase!NdrExtInterfacePointerUnmarshall(_MIDL_STUB_MESSAGE*, unsigned char**, unsigned char const*)+0x1cd
18 rpcrt4!NdrPointerUnmarshall+0x264
19 rpcrt4!NdrPointerUnmarshall+0x3c4
20 rpcrt4!NdrpClientUnMarshal+0x3ec
21 rpcrt4!union _CLIENT_CALL_RETURN NdrpClientCall2(struct _MIDL_STUB_DESC const *,unsigned char const *,unsigned char *,unsigned char)+0x5e2
22 combase!ObjectStublessClient(void*, long long*, long)+0x1d8
23 combase!ObjectStubless()+0x42
24 wbemprox!long CDCOMTrans::DoActualConnection(unsigned short *,unsigned short *,unsigned short *,unsigned short *,long,unsigned short *,struct IWbemContext *,struct IWbemServices * *)+0x5dc
25 wbemprox!virtual long CLocator::ConnectServer(unsigned short * const,unsigned short * const,unsigned short * const,unsigned short * const,long,unsigned short * const,struct IWbemContext *,struct IWbemServices * *)+0x112
26 xul!AnnotateWMIData_ThreadStart(void*)+0xe0
27 nss3!_PR_NativeRunThread(void*)+0x14a

Comment 1

[Toshihito Kikuchi [:toshi]]

This task is the same as what is reported as a security issue (bug 1692967), but I filed a new one because we concluded there is no security risk and we should do this from a different perspective.

Comment 2

[Toshihito Kikuchi [:toshi]]

@David and @Gabriele, do you know anybody monitoring MemoryErrorCorrection in the crash reports?

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

I have used the MemoryErrorCorrection flag occasionally to be sure that I was looking at a real crash and not something caused by bad memory. It's not something I do often though. Part of why we did this was because we wanted to compare crash rates of machines with ECC memory vs machines w/o them. See the comments in bug 1498609. Also see bug 1489297 comment 0. If this is problematic then removing it would have a small impact, but I would be happy if we could keep it w/o using WMI. BIOS manufacturer can definitely go, we don't care especially since Microsoft started shipping microcode updates as part of Windows.

Comment 4

[Toshihito Kikuchi [:toshi]]

Thanks for that info. There is no strong reason to remove the code, so I'm fine with keeping WMI code as is.

Part of why we did this was because we wanted to compare crash rates of machines with ECC memory vs machines w/o them.

I'm just curious about this. Do we have a query to monitor this comparison?

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Toshihito Kikuchi [:toshi] from comment #4)

I'm just curious about this. Do we have a query to monitor this comparison?

Not yet, but it would be interesting to have one.

Comment 6

[Toshihito Kikuchi [:toshi]]

Resolving this as Wontfix as there is no action item on this. Thank you for clarifying the current status, Gabriele.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

I'm very much in favor of removing the BIOS Manufacturer annotation - which I think is useless at this point. If it benefits us from a performance and security perspective - as you mentioned in comment 0 - I'm OK with removing MemoryErrorCorrection too. It's unused now so I'd rather pick the low hanging fruit than wait for something to come to fruition at a later date. We can reinstate MemoryErrorCorrection at a later point through a better mechanism: gathering the annotations in a separate process come to mind, and only doing so when Firefox crashes would be even better. WDYT?

Comment 8

[Toshihito Kikuchi [:toshi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #7)

gathering the annotations in a separate process come to mind, and only doing so when Firefox crashes would be even better. WDYT?

That's a great idea. I'm wondering if crashreporter.exe can do it? Anyway, let me reopen the bug and prepare a patch to remove the WMI code.

Comment 9

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1710152 - Remove the BIOS_Manufacturer and MemoryErrorCorrection annotations. r=gsvelto

The brower process used WMI to add BIOS Manufacturer and MemoryErrorCorrection
annotations to a crash report. The downside is that calling IWbemLocator::ConnectServer
loads all of the registered AMSI modules into the process. As we don't actively use either
of the annotations now, this patch removes WMI code to improve the stability.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f459092d4d76
Remove the BIOS_Manufacturer and MemoryErrorCorrection annotations. r=gsvelto

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/f459092d4d76