1.68 KB, patch
|Details | Diff | Splinter Review|
09:46:18 INFO - TEST-FAILED | WindowsDllInterceptor | Failed to execute hooked function SetCursorPos from user32.dll Perhaps the cursor is changing between GetCursorPos and SetCursorPos? I wonder if it would make more sense to just give SetCursorPos garbage and assert that it fails.
Created attachment 8872779 [details] [diff] [review] ignoreReturn.patch > Perhaps the cursor is changing between GetCursorPos and SetCursorPos? > > I wonder if it would make more sense to just give SetCursorPos garbage and > assert that it fails. Actually, its the SetCursorPos call that is failing -- the test shouldn't care if the cursor changes positions between calls. Indeed, docs don't mention how SetCursorPos _can_ fail (it takes 2 ints as position and clips them if they are out of bounds so all inputs are valid). Usually, this means a security failure. Rather than care what the answer is, I've removed the return value check. The tests are really intended to be a sanity check that we didn't write garbage assembler into the harness. Their failures are almost certainly going to be crashes. (You'll see other methods in there where I don't check the return value for because they were also problematic.)
Attachment #8872779 - Flags: review?(dmajor)
Comment on attachment 8872779 [details] [diff] [review] ignoreReturn.patch Oh, if all inputs are valid then perhaps we should just pass 512,512 all the time?
Attachment #8872779 - Flags: review?(dmajor) → review+
Created attachment 8873131 [details] [diff] [review] Ignore return value in SetCursorPos test (r=dmajor) I've removed the GetCursorPos call and just used 512,512 as you suggested. I was resisting that as we've now gone full circle back to my _original_ implementation in the old bug... but the even mild additional complexity wasn't actually warranted.
Attachment #8872779 - Attachment is obsolete: true
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d99ba297f Ignore return value of SetCursorPos in TestDllInterceptor. r=dmajor
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.