648581 - If DLL blocklisting hook fails, annotate crash reports

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should annotate crash reports if the DLL blocklist fails.  This is tricky since the DLL blocklist code runs before XPCOM init.

Comment 1

[(no longer active)]

When you're talking about blocklisting failing, are you talking about the possible failure to install the patched LdrLoadDll, or failures inside patched_LdrLoadDll (or both?)

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I really was only considering the first.  Idk if there's a way to get at the second.

I noticed everytime I run a debug build I get the LdrLoadDll hook failed warning, so I was curious how many of our users see the same thing.

Comment 3

[(no longer active)]

(In reply to comment #2)
> I really was only considering the first.  Idk if there's a way to get at the
> second.
> 
> I noticed everytime I run a debug build I get the LdrLoadDll hook failed
> warning, so I was curious how many of our users see the same thing.

I guess the most worrying thing would be the first one.  But the more interesting thing is to see why this is failing on your machine.  Can you debug that, please?

Comment 4

[(no longer active)]

Attachment: Part 1: Make nsWindowsDllBlocklist.h useful

I've always found the way that nsWindowsDllBlocklist.h works rather stupid.  It's not useful as a header, and so far its purpose has been to split nsWindowsDllBlocklist.cpp into two files, for no good reason.

This patch moves all the contents into the cpp file, to enable us to actually use the header.

Comment 5

[(no longer active)]

The actual patch is tricky, since the data indicating whether the operation was successful lives in firefox.exe, and the code which needs to query it lives in xul.dll, and I don't really want to export a symbol from firefox.exe to gain access to that data...

I'm not sure what the best solution to this problem is..

Comment 6

[Benjamin Smedberg]

Comment on attachment 526268 [details] [diff] [review]
Part 1: Make nsWindowsDllBlocklist.h useful

Export an XRE_ function?

Comment 7

[(no longer active)]

Attachment: Do the real work

This is a sample of how reports would look like with this patch:

https://crash-stats.mozilla.com/report/index/c70a6da0-ad5c-4eb1-8352-cd6692110502

(I had to inject a failure in the blocklist setup code manually.)

Comment 8

[(no longer active)]

Attachment: Rolled up patch

This is a rolled up version, because it turns out that separating part 1 didn't make sense.  But attachment 529505 [details] [diff] [review] should be easier to review.

Comment 9

[Benjamin Smedberg]

Comment on attachment 529505 [details] [diff] [review]
Do the real work

glandium is moving the DLL blocklist into libxul in bug 552864, in which case we really don't need or want the XRE_ functions here. Can this wait for that change and then we can just use internal functions?

Comment 10

[(no longer active)]

It can wait, I guess, unless Kyle wants it in sooner.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not in any particular hurry.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, Bug 552864 is fixed now.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ehsan, do you have time to finish this?

Comment 14

[(no longer active)]

Attachment: New patch

Sorry for the delay.

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a6fdf0144

Comment 16

[Ed Morley [:emorley]]

Attachment: Followup fix for --disable-crashreporter

Bustage fix for using --disable-crashreporter

{
nsWindowsDllBlocklist.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl CrashReporter::AppendAppNotesToCrashReport(class nsACString_internal const &)" (?AppendAppNotesToCrashReport@CrashReporter@@YAIABVnsACString_internal@@@Z) referenced in function _XRE_SetupDllBlocklist
xul.dll : fatal error LNK1120: 1 unresolved externals
}

Comment 17

[Ed Morley [:emorley]]

Comment on attachment 561090 [details] [diff] [review]
Followup fix for --disable-crashreporter

rs=glandium via #pymake

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab120ebd437b

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ab0a6fdf0144
https://hg.mozilla.org/mozilla-central/rev/ab120ebd437b

(Thanks Ed for fixing the --disable-crashreporter case)