Closed Bug 1368033 Opened 3 years ago Closed 3 years ago

Intermittent TestDllInterceptor.exe | test failed with return code 1

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: handyman)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(davidp99)
Attached patch ignoreReturn.patch (obsolete) — Splinter Review
> 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.)
Flags: needinfo?(davidp99)
Attachment #8872779 - Flags: review?(dmajor)
Assignee: nobody → davidp99
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+
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 ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d99ba297f
Ignore return value of SetCursorPos in TestDllInterceptor. r=dmajor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a12d99ba297f
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.