Closed Bug 1192844 Opened 5 years ago Closed 5 years ago

[Cpp] Failed TestDllInterceptor on windows 10 (64 bit) Failed to hook SetWindowLongPtrA from user32.dll

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: Callek, Assigned: dmajor)

References

Details

Attachments

(1 file, 1 obsolete file)

22:11:05     INFO -  TEST-START | TestDllInterceptor.exe
22:11:05     INFO -  PROCESS | 1228 |
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Hook added
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Hook called
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Hook works properly
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Hook was not called after unregistration
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Original function worked properly
22:11:05     INFO -  TEST-PASS | WindowsDllInterceptor | Could hook GetWindowInfo from user32.dll
22:11:05  WARNING -  TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to hook SetWindowLongPtrA from user32.dll
22:11:05  WARNING -  TEST-UNEXPECTED-FAIL | TestDllInterceptor.exe | test failed with return code 1
22:11:05     INFO -  TEST-INFO took 13ms

(full log: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-d4b5af30bd85/try-win64-debug/try_win10_64-debug_test-cppunit-bm109-tests1-windows-build0.txt.gz )


:dolske, this is the only failure on Cpp for both opt and debug, can you recommend an owner for this?
Flags: needinfo?(dolske)
I can!
Assignee: nobody → dmajor
Flags: needinfo?(dolske)
On Win10 x64, SetWindowLongPtrA is just a stub with a mov and jmp (total 11 bytes) followed by some "int 3" padding. Those 11 bytes aren't long enough for our trampoline. If we stomped on some of the "int 3" then we'd have enough space.

I hesitate to decode "int 3" unconditionally; it might be a debugger-inserted breakpoint. But I think it would be okay to decode an "int 3" if it is followed by another one. Then we can be reasonably sure it's just padding.
Attached patch Accept 0xCC padding (obsolete) — Splinter Review
Callek can you give this a try?
Attachment #8645771 - Flags: feedback?(bugspam.Callek)
The test still fails because I tripped over this code:

      // if found JMP 32bit offset, next bytes must be NOP
      if (pJmp32 >= 0) {
        if (origBytes[nBytes++] != 0x90) {
          return;
        }

        continue;
      }
This should do better
Attachment #8645771 - Attachment is obsolete: true
Attachment #8645771 - Flags: feedback?(bugspam.Callek)
(In reply to David Major [:dmajor] from comment #6)
> Created attachment 8645942 [details] [diff] [review]
> Accept 0xCC padding
> 
> This should do better

New push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db63cc322c1
Comment on attachment 8645942 [details] [diff] [review]
Accept 0xCC padding

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

This yielded Green Cpp on windows 10
Attachment #8645942 - Flags: feedback+
Attachment #8645942 - Flags: review?(m_kato)
Attachment #8645942 - Flags: review?(m_kato) → review+
Jim, this bug means that the SetWindowLongPtrA hooks (for a Flash fix, I think?) are broken on Win10 x64. Do you think we should uplift the fix?
Flags: needinfo?(jmathies)
https://hg.mozilla.org/mozilla-central/rev/5e520f58ae39
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(In reply to David Major [:dmajor] from comment #10)
> Jim, this bug means that the SetWindowLongPtrA hooks (for a Flash fix, I
> think?) are broken on Win10 x64. Do you think we should uplift the fix?

Not too worried about SetWindowLongPtr*A* intercepts.

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowWin.cpp#363

363  * Note, ascii versions can be nixed once flash versions < 10.1
364  * are considered obsolete.
Flags: needinfo?(jmathies)
See Also: → 1342690
You need to log in before you can comment on or make changes to this bug.