The default bug view has changed. See this FAQ.

Add some additional instructions to x64 detours patcher

RESOLVED FIXED in Firefox 52

Status

()

Core
General
RESOLVED FIXED
a year ago
6 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)

(Assignee)

Description

a year ago
It can't handle CreateWindowExW
(Assignee)

Updated

a year ago
Blocks: 1240320
status-firefox45: --- → affected
[Tracking Requested - why for this release]: Merge day bustage.
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
(Assignee)

Comment 2

a year ago
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!
(Assignee)

Comment 3

a year ago
Created attachment 8709654 [details] [diff] [review]
Patch
Attachment #8709654 - Flags: review?(ehsan)
(Assignee)

Comment 4

a year ago
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+
(Assignee)

Comment 6

a year ago
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+
(Assignee)

Comment 7

a year ago
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

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a786af9186eb
(Assignee)

Comment 9

a year ago
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?

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a786af9186eb
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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)
(Assignee)

Comment 12

a year ago
I suppose not, though on 32-bit it'll just do a nop-space patch. I can land a follow-up.
Flags: needinfo?(aklotz)
(Assignee)

Comment 13

a year ago
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-
(Assignee)

Comment 15

a year ago
(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)
(Assignee)

Comment 16

a year ago
(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
(Assignee)

Comment 22

a year ago
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Cancelling this for now
Attachment #8709745 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

a year ago
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)
(Assignee)

Comment 24

a year ago
This is the try push for that patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abcc0535e30c
Attachment #8716088 - Flags: review?(ehsan) → review+
(Assignee)

Comment 25

a year ago
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

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4e5e584c32
(Assignee)

Comment 27

a year ago
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?
(Assignee)

Comment 28

a year ago
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?

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e4e5e584c32
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year 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+

Comment 31

a year ago
bugherderuplift
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

a year 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)
(Assignee)

Comment 42

a year ago
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)
(Assignee)

Updated

a year ago
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
(Assignee)

Comment 46

6 months ago
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+
(Assignee)

Comment 47

6 months ago
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

Comment 48

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/609a153be0a5
Status: REOPENED → RESOLVED
Last Resolved: a year ago6 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.