1240848 - Add some additional instructions to x64 detours patcher

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

It can't handle CreateWindowExW

Comment 1

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Merge day bustage.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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!

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

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 5

[(no longer active)]

Comment on attachment 8709654 [details] [diff] [review]
Patch

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

Thanks!  Please add a test too.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch (r2)

Added test as requested, carrying forward r+

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch (inbound)

Patch (r2) is for Aurora 45. This one is rebased to inbound 46. Carrying forward r+.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a786af9186eb

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a786af9186eb

Comment 11

[(no longer active)]

Is there any reason to keep the test specific to 64-bit? <https://hg.mozilla.org/mozilla-central/rev/a786af9186eb#l1.12>

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

I suppose not, though on 32-bit it'll just do a nop-space patch. I can land a follow-up.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Actually I should force a detour in the 32-bit test case.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

(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?

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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

Fix crashes, taking it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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

oups, sorry

Comment 20

[(no longer active)]

(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!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

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

Cancelling this for now

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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.

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

This is the try push for that patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abcc0535e30c

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch (for inbound and aurora)

This patch is the same as beta but merged for inbound / aurora landing.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4e5e584c32

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e4e5e584c32

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Fix a crash, taking.
Should be in 45 beta 5.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d6412cfea15

Comment 32

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/4febbf1c7a8e

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like this relanding caused bug 1241921 to return on nightly.

Comment 34

[Carsten Book [:Tomcat]]

backedout from mozilla-central and will back this also out from aurora/beta for comment #33

Comment 35

[Carsten Book [:Tomcat]]

backed out from beta and aurora now too

Comment 36

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/904f3554c084

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The aurora and beta backouts are:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4ba2ee01bf5
https://hg.mozilla.org/releases/mozilla-beta/rev/9321c2755a3a

Comment 38

[Gian-Carlo Pascutto [:gcp]]

I suspect my bustage today is related to this, bug 1247741.

Comment 39

[Carsten Book [:Tomcat]]

in danger of missing the beta cycle

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Benjamin, do we care about this with async plugin=off?

Comment 41

[Benjamin Smedberg]

Redirecting to aklotz. My understanding was that this patch was required to get correct Flash behavior on x86-64, but he's the expect.

Comment 42

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

OK, let's wontfix it for 45 & 46 then.

Comment 45

[Chris Peterson [:cpeterson]]

Aaron recommends that this bug block the 64-bit Firefox roll-out.

Comment 46

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch rebased for 52

Rebased patch, carrying forward r+.

Comment 47

[(No longer employed by Mozilla) Aaron Klotz]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/609a153be0a5