I'm looking for somebody to take this.

Some additional info:

• The issue is with TestDllInterceptor.exe (not TestDllInterceptorCrossProcess.exe).
• You can run the TestDllInterceptor.exe in dist/bin by copying mozglue.dll from dist/mozglue to dist/bin and installing VCRuntime from [1].
• The disease has spread since this report. Using Windows 8.1 x64, I am seeing additional failures. Remembering that the trampoline needs 13 bytes, the offenders and their x64 machine codes are:
  • CloseHandle: ff 25 ca 21 12 00 90 90 90 90 90 90 ff 25 1e 1a
    • The second rip-relative jmp starts at byte 13. CloseHandle could work as a 10-byte trampoline. I don't see another safe solution at this point.
  • SetWindowLongPtrA: 41 b9 01 00 00 00 e9 a5 f8 ff ff 41 b8 ff ff 00
    • The issue again is instructions in the first 13 bytes that follow a jmp.
    • The disassembly is:
      0x0000000000000000: 41 B9 01 00 00 00 mov r9d, 1
      0x0000000000000006: E9 A5 F8 FF FF jmp 0xfffffffffffff8b0
      [...]
      so the "41 b8" bytes follow the jmp and are in the trampoline space. Again, this could work with a 10-byte trampoline.
  • SetWindowLongPtrW: 45 33 c9 e9 48 ff ff ff b2 01 e8 71 fe ff ff 48
    • Disassembly:
      0x0000000000000000: 45 33 C9 xor r9d, r9d
      0x0000000000000003: E9 48 FF FF FF jmp 0xffffffffffffff50
      0x0000000000000008: B2 01 mov dl, 1
      0x000000000000000a: E8 71 FE FF FF call 0xfffffffffffffe80
    • I'm at a loss as to what to do here. We can only handle the first 8 bytes so even the 10-byte patch would fail. Presumably something jmps to the mov at 0x8 or else it would be worthless, so its not ok to overwrite it.
    • The usage is for crash-protecting Flash WndProc.

I'm attaching a patch that forces 10-byte patches for CloseHandle and SetWindowLongPtrA. I think this is the right solution in those cases; it fixes them in the test. For SetWindowLongPtrW, I need some advice -- NI to Aaron.

[1] https://www.microsoft.com/en-us/download/details.aspx?id=52685

For SetWindowLongPtrW, I need some advice -- NI to Aaron.

For however long this hasn't been working, are we seeing a significant number of crashes in the wild due to our inability to set this hook? If not, maybe we should just skip this test on Win8.1.

Otherwise my thought is that we leverage some of the stuff I wrote for ARM64 where we find a memory region within the range of an int32 displacement and set up a veneer, however that seems like a bit much work for something that might not matter all that much.

I'm going to dig into why we are hooking both SetWindowLongPtrW and SetWindowLongPtrA to try to find a way to avoid SetWindowLongPtrW. Maybe we'll get lucky and find that Flash isn't using it anymore.

I've spent a day looking at this with Flash and I have not found any uses of SetWindowLongPtrW (or SetWindowLongPtrA for that matter). I've tried with/without async rendering, normal and fullscreen, wmode as unspecified and as "window" [1]. While its possible that some untested behavior triggers this, I am nearly certain that its just part of the old windowed handlers. If that is the case then we can stop hooking both of these -- but if we don't want to find out I'm wrong about this, we could just cut the hooks in Win 8. The rest tests fine now and should get dumped soon anyway.

This is the relevant hook [2]. NI to jimm to see if he can confirm that the crash-protected window handler is not used e.g. for invisible Flash windows. Unless jimm thinks it is in use, I'm going to suggest we skip this in Win 8 only and hook the other 2 with 10-byte patches as in the wip above.

--

[1] https://wiki.mozilla.org/Plugins/Async_Drawing
[2] https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/dom/plugins/ipc/PluginInstanceChild.cpp#1825

(If jimm is certain that we don't use them then lets totally rip them out.)

(In reply to David Parks (dparks) [:handyman] from comment #7)

I'm going to dig into why we are hooking both SetWindowLongPtrW and SetWindowLongPtrA to try to find a way to avoid SetWindowLongPtrW. Maybe we'll get lucky and find that Flash isn't using it anymore.

We removed all the windowing modes, so I think we can rip all of this subclass code out (or maybe just disable it). The SetWindowLongPtrA calls we're for an older version of flash as well, which is blocked. So we don't need those hooks either.

That's the same conclusion I came to. I'll post a patch today.

CloseHandle has a jump followed by enough nops to fit a 10-byte patch but not enough to fit the default 13-byte patch when running Windows 8 or 8.1. This patch tells the interceptor to use a 10-byte patch on those OSs.

SetWindowLong*/SetWindowLongPtr* was being intercepted so that we could override windowprocs in windowed plugins on Windows. We no longer support windowed plugins so these functions are never intercepted.

Depends on D55535

https://hg.mozilla.org/mozilla-central/rev/bdffefea6e9a
https://hg.mozilla.org/mozilla-central/rev/33fef95ff8cf

With our routine verification on the DLL Interceptors, starting with 69.0b16-build1 this issue was no longer encountered and reported.
A more detailed report can be found here.

Marking the issue as verified with 73.0b1-build2, as our latest verified version.