If DLL blocklisting hook fails, annotate crash reports

RESOLVED FIXED in mozilla9

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: Ehsan)

Tracking

unspecified
mozilla9
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

We should annotate crash reports if the DLL blocklist fails.  This is tricky since the DLL blocklist code runs before XPCOM init.
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?)
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.
(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?
Created attachment 526268 [details] [diff] [review]
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #526268 - Flags: review?(benjamin)
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 on attachment 526268 [details] [diff] [review]
Part 1: Make nsWindowsDllBlocklist.h useful

Export an XRE_ function?
Attachment #526268 - Flags: review?(benjamin) → review+
Created attachment 529505 [details] [diff] [review]
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.)
Attachment #529505 - Flags: review?(benjamin)
Created attachment 529609 [details] [diff] [review]
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 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?
It can wait, I guess, unless Kyle wants it in sooner.
Depends on: 552864
I'm not in any particular hurry.
Attachment #529505 - Flags: review?(benjamin)
So, Bug 552864 is fixed now.
Ehsan, do you have time to finish this?
Created attachment 560843 [details] [diff] [review]
New patch

Sorry for the delay.
Attachment #526268 - Attachment is obsolete: true
Attachment #529505 - Attachment is obsolete: true
Attachment #529609 - Attachment is obsolete: true
Attachment #560843 - Flags: review?(benjamin)
Attachment #560843 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a6fdf0144
Target Milestone: --- → mozilla9
Created attachment 561090 [details] [diff] [review]
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
}
Attachment #561090 - Flags: review?(benjamin)
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
Attachment #561090 - Flags: review?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/ab0a6fdf0144
https://hg.mozilla.org/mozilla-central/rev/ab120ebd437b

(Thanks Ed for fixing the --disable-crashreporter case)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 688357
You need to log in before you can comment on or make changes to this bug.