Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add some additional instructions to x64 detours patcher

RESOLVED FIXED in Firefox 52

Status

()

Core
General
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45+ wontfix, firefox46+ wontfix, firefox47 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

It can't handle CreateWindowExW
Blocks: 1240320
status-firefox45: --- → affected
[Tracking Requested - why for this release]: Merge day bustage.
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
Also: there's a problem with the x64 detours patcher where it will clobber r11 for a jmp even if the code in the trampoline is already using r11!

eg in the CreateWindowExW case, the trampoline code is already using r11 and eax, leaving r10 as the only volatile register available for writing without harming correctness!
Created attachment 8709654 [details] [diff] [review]
Patch
Attachment #8709654 - Flags: review?(ehsan)
I opted to use a different type of jmp to prevent r11 clobbering. It was either that or add a bunch of bookkeeping to see which volatile registers had been clobbered in the trampoline.
Comment on attachment 8709654 [details] [diff] [review]
Patch

Review of attachment 8709654 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Please add a test too.
Attachment #8709654 - Flags: review?(ehsan) → review+
Created attachment 8709745 [details] [diff] [review]
Patch (r2)

Added test as requested, carrying forward r+
Attachment #8709654 - Attachment is obsolete: true
Attachment #8709745 - Flags: review+
Created attachment 8709750 [details] [diff] [review]
Patch (inbound)

Patch (r2) is for Aurora 45. This one is rebased to inbound 46. Carrying forward r+.
Attachment #8709750 - Flags: review+

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a786af9186eb
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugin async init
[User impact if declined]: Assertions, crashes, bad things on x64 with async init enabled
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8709745 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a786af9186eb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Is there any reason to keep the test specific to 64-bit? <https://hg.mozilla.org/mozilla-central/rev/a786af9186eb#l1.12>
Flags: needinfo?(aklotz)
I suppose not, though on 32-bit it'll just do a nop-space patch. I can land a follow-up.
Flags: needinfo?(aklotz)
Actually I should force a detour in the 32-bit test case.
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

This makes RELEASE_BUILD angry. The latest Try push shows mass "application terminated with exit code 3221225477" test failures, and I can reproduce that locally when running the Try build via mozregression as well. On my end, it appeared to be a shutdown crash, though.
Attachment #8709745 - Flags: feedback-
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Comment on attachment 8709745 [details] [diff] [review]
> Patch (r2)
> 
> This makes RELEASE_BUILD angry. The latest Try push shows mass "application
> terminated with exit code 3221225477" test failures, and I can reproduce
> that locally when running the Try build via mozregression as well. On my
> end, it appeared to be a shutdown crash, though.

Was this only on 64-bit builds, by any chance?
Flags: needinfo?(ryanvm)
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #15)
> Was this only on 64-bit builds, by any chance?

n/m, of course it is. This is interesting -- I was experiencing the same problem briefly when I first wrote the Optimus detection patch. I'm kind of worried that something bad is happening in the interceptor's destructor.
Flags: needinfo?(ryanvm)
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Fix crashes, taking it.
Attachment #8709745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox45: ? → +
tracking-firefox46: ? → +
I don't think we should take this patch on Aurora until comments 14+ are addressed or we're going to have across the board Win64 permafail on Beta after Monday's uplift.
Flags: needinfo?(sledru)
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

oups, sorry
Flags: needinfo?(sledru)
Attachment #8709745 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #12)
> I suppose not, though on 32-bit it'll just do a nop-space patch. I can land
> a follow-up.

Yes please!
Backed out along with bug 1240977 under the suspicion that this was the cause of bug 1241921.
https://hg.mozilla.org/mozilla-central/rev/5f7c184ccd80
Status: RESOLVED → REOPENED
status-firefox46: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla46 → ---
Depends on: 1241921
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Cancelling this for now
Attachment #8709745 - Flags: approval-mozilla-aurora?
Created attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Resubmitting for review. The main change from the previous rev is the writing out of trampoline bytes when pJmp32 >= 0. I was using nBytes as the base index when I should have been using pJmp32 as the base index.
Attachment #8709745 - Attachment is obsolete: true
Attachment #8716088 - Flags: review?(ehsan)
This is the try push for that patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abcc0535e30c
Attachment #8716088 - Flags: review?(ehsan) → review+
Created attachment 8717085 [details] [diff] [review]
Patch (for inbound and aurora)

This patch is the same as beta but merged for inbound / aurora landing.
Attachment #8709750 - Attachment is obsolete: true
Attachment #8717085 - Flags: review+

Comment 26

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4e5e584c32
Comment on attachment 8717085 [details] [diff] [review]
Patch (for inbound and aurora)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugins
[User impact if declined]: Assertions, crashes, bad things on x64
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8717085 - Flags: approval-mozilla-aurora?
Comment on attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugins
[User impact if declined]: Assertions, crashes, bad things on x64
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8716088 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/6e4e5e584c32
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Fix a crash, taking.
Should be in 45 beta 5.
Attachment #8716088 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8717085 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d6412cfea15
status-firefox46: affected → fixed
 https://hg.mozilla.org/releases/mozilla-beta/rev/4febbf1c7a8e
status-firefox45: affected → fixed
It looks like this relanding caused bug 1241921 to return on nightly.
backedout from mozilla-central and will back this also out from aurora/beta for comment #33
Status: RESOLVED → REOPENED
Flags: needinfo?(aklotz)
Resolution: FIXED → ---
backed out from beta and aurora now too
status-firefox45: fixed → affected
status-firefox46: fixed → affected
status-firefox47: fixed → affected

Comment 36

2 years ago
Backout:
https://hg.mozilla.org/mozilla-central/rev/904f3554c084
The aurora and beta backouts are:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4ba2ee01bf5
https://hg.mozilla.org/releases/mozilla-beta/rev/9321c2755a3a
I suspect my bustage today is related to this, bug 1247741.
in danger of missing the beta cycle
Flags: needinfo?(sledru)
Benjamin, do we care about this with async plugin=off?
Flags: needinfo?(sledru) → needinfo?(benjamin)
Redirecting to aklotz. My understanding was that this patch was required to get correct Flash behavior on x86-64, but he's the expect.
Flags: needinfo?(benjamin)
It's less likely to be an issue with async init off, but I think to be certain we should throw some QA at it on Win64. Basically general Flash usage, making sure there aren't any hangs or stability issues.

FWIW I don't think I can just pull a solution out of a hat on this one -- these NVIDIA crashes that are induced by these changes are very tricky to debug.
Flags: needinfo?(aklotz)
Duplicate of this bug: 1241921
OK, let's wontfix it for 45 & 46 then.
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
Aaron recommends that this bug block the 64-bit Firefox roll-out.
Blocks: 558448
Created attachment 8800896 [details] [diff] [review]
Patch rebased for 52

Rebased patch, carrying forward r+.
Attachment #8716088 - Attachment is obsolete: true
Attachment #8717085 - Attachment is obsolete: true
Attachment #8800896 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/609a153be0a59c7cdb79d06c0254dfdeb33dbaa4
Bug 1240848: Adds additional instructions to x64 detour patcher and prevents register clobbering in jmp from trampoline; r=ehsan
https://hg.mozilla.org/mozilla-central/rev/609a153be0a5
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago9 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.