Closed
Bug 982077
Opened 11 years ago
Closed 11 years ago
Set up DLL blocklist before LoadAppInitDlls (Port Bug 932100 to Thunderbird)
Categories
(Thunderbird :: OS Integration, defect)
Tracking
(thunderbird29+ fixed, thunderbird30+ fixed)
RESOLVED
FIXED
Thunderbird 31.0
People
(Reporter: Irving, Assigned: standard8)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
Irving
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #947599 +++
From: Bug 932100 - Set up DLL blocklist before LoadAppInitDlls
> The DLL blocklist is ineffective against AppInit DLLs because they get
> loaded before the DLL blocklist is initialized. Bug 925459 is an example.
>
> kernel32!LoadAppInitDlls is called during user32's DLL_PROCESS_ATTACH. In
> order to intercept AppInit DLLs, we would need to hook LdrLoadDll before
> user32 gets loaded.
>
> Currently, firefox.exe has a load-time link to user32.dll, which means we do
> LoadAppInitDlls before any Mozilla code is executed. It turns out that this
> link to user32 is only used by one call to MessageBoxW from an error handler
> in nsBrowserApp.cpp. Given the rarity of that codepath, we could easily turn
> it into a runtime dynamic link.
>
> That would at least let us run firefox!wmain before user32, but the
> blocklist is implemented in xul.dll, and by the time we load that,
> InitXPCOMGlue has already loaded a ton of stuff, including user32.
>
> Some potential options:
> 1) Have a separate blocklist in firefox.exe for AppInit DLLs
> 2) Move the blocklist from xul.dll to firefox.exe
> 3) Disentangle things and try to get xul!XRE_SetupDllBlocklist available
> before InitXPCOMGlue preloads the world
Please note that this change has caused many headaches in FF, due to unforeseen interactions with external applications. Bug 951827 tracks the ongoing attempts to mitigate. Technical details of the problem are in bug 951827 comment 15.
Assignee | ||
Comment 2•11 years ago
|
||
I've ported bug 762310, part of bug 755724, bug 932100, and pushed these to try server:
https://hg.mozilla.org/try-comm-central/rev/3a14b5636cd6
Hopefully the external application issues mentioned in comment 1 are now long resolved and we won't have too much of an issue.
(Picking up as this could have caused bug 990676).
Assignee: nobody → standard8
> Hopefully the external application issues mentioned in comment 1 are now
> long resolved and we won't have too much of an issue.
After several attempts, those issues were finally resolved by Attachment #8383957 [details] [diff]. You may want to pick up that as well.
Assignee | ||
Comment 4•11 years ago
|
||
Try server builds ended up here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=77d5bce20e8b
Using the crashme extension (https://code.google.com/p/crashme/) the try server build crashes & produces the crash reporter and submits it to crash-stats.
When I had tried one build previously (I forget which, probably beta), Thunderbird just hung when hitting the crash me button.
This ports bug 762310, part of bug 755724, bug 932100 - not all of these are required, but they got out app file a bit closer to Firefox's in the areas we need them to be.
Attachment #8404634 -
Flags: review?(irving)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to David Major [:dmajor] (UTC+12) from comment #3)
> > Hopefully the external application issues mentioned in comment 1 are now
> > long resolved and we won't have too much of an issue.
>
> After several attempts, those issues were finally resolved by Attachment
> #8383957 [details] [diff]. You may want to pick up that as well.
That's all toolkit/core so we've already picked those up.
Reporter | ||
Comment 6•11 years ago
|
||
Comment on attachment 8404634 [details] [diff] [review]
The fix
Review of attachment 8404634 [details] [diff] [review]:
-----------------------------------------------------------------
I really wish we had an automated test for this, but the FF bugs don't appear to have one...
::: mail/app/nsMailApp.cpp
@@ +165,5 @@
> +#ifdef DEBUG
> + // In order to be effective against AppInit DLLs, the blocklist must be
> + // initialized before user32.dll is loaded into the process (bug 932100).
> + if (GetModuleHandleA("user32.dll")) {
> + fprintf(stderr, "DLL blocklist was unable to intercept AppInit DLLs.\n");
This is a MOZ_ASSERT in the Firefox version; do we really want to downgrade to just a printf?
Attachment #8404634 -
Flags: review?(irving) → review+
Comment 7•11 years ago
|
||
(In reply to :Irving Reid from comment #6)
> I really wish we had an automated test for this, but the FF bugs don't
> appear to have one...
I was thinking the same thing.
I am certain we had (manual) tests in litmus. I am unsure if a crash test is on usul's release checklist and in the newer moztap. ludo?
Flags: needinfo?(ludovic)
Comment 8•11 years ago
|
||
(In reply to :Irving Reid from comment #6)
> Comment on attachment 8404634 [details] [diff] [review]
> The fix
>
> Review of attachment 8404634 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I really wish we had an automated test for this, but the FF bugs don't
> appear to have one...
>
> ::: mail/app/nsMailApp.cpp
> @@ +165,5 @@
> > +#ifdef DEBUG
> > + // In order to be effective against AppInit DLLs, the blocklist must be
> > + // initialized before user32.dll is loaded into the process (bug 932100).
> > + if (GetModuleHandleA("user32.dll")) {
> > + fprintf(stderr, "DLL blocklist was unable to intercept AppInit DLLs.\n");
>
> This is a MOZ_ASSERT in the Firefox version; do we really want to downgrade
> to just a printf?
Yes. See Bug 939043 - Downgrade the user32 assertion to a warning message
https://hg.mozilla.org/mozilla-central/rev/c149ae799213
Assignee | ||
Comment 9•11 years ago
|
||
As Philip says, the MOZ_ASSERT was changed in a later bug/commit, so we do want that.
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/7ef9725513af
Lets hope that fixes the crash one as well.
Status: NEW → RESOLVED
Closed: 11 years ago
tracking-thunderbird31:
+ → ---
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8404634 [details] [diff] [review]
The fix
[Triage Comment]
I've proven this fixes the crash issue on windows, so taking across to aurora & beta for more testing.
Attachment #8404634 -
Flags: approval-comm-beta+
Attachment #8404634 -
Flags: approval-comm-aurora+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/ebef804039b7
https://hg.mozilla.org/releases/comm-beta/rev/d4bd2c2dd216
status-thunderbird29:
--- → fixed
status-thunderbird30:
--- → fixed
Updated•11 years ago
|
Flags: needinfo?(ludovic)
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•