Last Comment Bug 648581 - If DLL blocklisting hook fails, annotate crash reports
: If DLL blocklisting hook fails, annotate crash reports
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla9
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on: 552864
Blocks: 688357
  Show dependency treegraph
 
Reported: 2011-04-08 10:48 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-09-21 19:43 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Make nsWindowsDllBlocklist.h useful (6.49 KB, patch)
2011-04-15 08:49 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Do the real work (4.37 KB, patch)
2011-05-02 10:15 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
Rolled up patch (10.16 KB, patch)
2011-05-02 16:49 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
New patch (8.88 KB, patch)
2011-09-18 20:37 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Followup fix for --disable-crashreporter (1.36 KB, patch)
2011-09-19 17:55 PDT, Ed Morley [:emorley]
no flags Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-08 10:48:42 PDT
We should annotate crash reports if the DLL blocklist fails.  This is tricky since the DLL blocklist code runs before XPCOM init.
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-14 17:21:54 PDT
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 [:khuey] (khuey@mozilla.com) 2011-04-14 17:54:02 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-14 21:16:12 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 08:49:10 PDT
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.
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 12:56:29 PDT
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 [:bsmedberg] 2011-04-29 12:38:52 PDT
Comment on attachment 526268 [details] [diff] [review]
Part 1: Make nsWindowsDllBlocklist.h useful

Export an XRE_ function?
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2011-05-02 10:15:20 PDT
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.)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2011-05-02 16:49:59 PDT
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 9 Benjamin Smedberg [:bsmedberg] 2011-05-11 07:14:55 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-05-11 17:32:14 PDT
It can wait, I guess, unless Kyle wants it in sooner.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-11 17:34:13 PDT
I'm not in any particular hurry.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-08 10:38:54 PDT
So, Bug 552864 is fixed now.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-19 10:27:19 PDT
Ehsan, do you have time to finish this?
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-18 20:37:52 PDT
Created attachment 560843 [details] [diff] [review]
New patch

Sorry for the delay.
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-19 13:22:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a6fdf0144
Comment 16 Ed Morley [:emorley] 2011-09-19 17:55:35 PDT
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
}
Comment 17 Ed Morley [:emorley] 2011-09-20 03:56:53 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:42:18 PDT
https://hg.mozilla.org/mozilla-central/rev/ab0a6fdf0144
https://hg.mozilla.org/mozilla-central/rev/ab120ebd437b

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

Note You need to log in before you can comment on or make changes to this bug.