1368033 - Intermittent TestDllInterceptor.exe | test failed with return code 1

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=102277145&repo=mozilla-central

https://queue.taskcluster.net/v1/task/HNgFOd4JQbmDSho5nCQsPg/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[(Away)]

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.

Comment 2

[David Parks [:handyman]]

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

Comment 3

[(Away)]

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?

Comment 4

[David Parks [:handyman]]

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

Comment 5

[David Parks [:handyman]]

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f83446f1f9a8ae79bdb8f4d480185c5009ebb859

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d99ba297f
Ignore return value of SetCursorPos in TestDllInterceptor. r=dmajor

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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