1426722 - Audit explicit checks against S_OK and use SUCCEEDED() or FAILED() instead

Status: ASSIGNED

(Core :: Widget: Win32, defect, P3)

Description

[:Gijs (he/him)]

(In reply to Chris Peterson [:cpeterson] from bug 1426719 comment #6)
> Someone should probably audit the other uses of bare S_OK and S_FALSE in
> mozilla-central and determine whether they should use the SUCCEEDED or
> FAILED macros instead. And perhaps add a mach lint check.

This seems like a great idea. Jim, who could pick this up in the new year?

Comment 1

[Chris Peterson [:cpeterson]]

Adding a regex lint check [1] is pretty easy. It should check for == and != before or after S_OK. S_FALSE is so uncommon that it's probably safe to assume that any use S_FALSE is correct.

Perhaps the linter could allow an extra set of parentheses around the S_OK condition (like `if ((hr == S_OK))`) to suppress the warning for legitimate uses of S_OK? Adding extra parentheses is a common pattern for suppressing compiler warnings about assignments in conditions or unreachable code.

[1] https://firefox-source-docs.mozilla.org/tools/lint/create.html

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Chris Peterson [:cpeterson] from comment #1)
> Adding a regex lint check [1] is pretty easy. It should check for == and !=
> before or after S_OK. S_FALSE is so uncommon that it's probably safe to
> assume that any use S_FALSE is correct.
> 
> Perhaps the linter could allow an extra set of parentheses around the S_OK
> condition (like `if ((hr == S_OK))`) to suppress the warning for legitimate
> uses of S_OK? Adding extra parentheses is a common pattern for suppressing
> compiler warnings about assignments in conditions or unreachable code.
> 
> [1] https://firefox-source-docs.mozilla.org/tools/lint/create.html

If we go the linter route, we absolutely need an escape hatch. And we definitely need to actually take a look at the context around any existing cases -- simply running a substitution script across the tree is unacceptable.

I will r- any patch in this bug that changes anything in ipc/mscom, because that code handles HRESULTS correctly and any comparisons to S_OK are done there for very specific reasons.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Add mach lint check for comparisons with S_OK instead of using SUCCEEDED()

Use SUCCEEDED() or FAILED() macro instead of comparing with S_OK because some APIs may return other non-error codes like S_FALSE. To suppress this warning, add extra parentheses around the expression like `((hr == S_OK))`.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 8938687 [details] [diff] [review]
Add mach lint check for comparisons with S_OK instead of using SUCCEEDED()

Aaron, here are some examples of suppressing the S_OK warnings in ipc/mscom. Do you think this `((hr == S_OK))` approach is too ugly?

If so, any suggestions for an alternate way to indicate the S_OK comparison is OK? Otherwise, I could just exclude the ipc/mscom directory from this lint.

Comment 5

[Mathew Hodson]

This should block bug 1027713 instead of depend on bug 1197049. It was that bug that added the checks for stream_set_volume in https://hg.mozilla.org/mozilla-central/rev/bf4ed2946c45

All bug 1197049 did was rename the function.

Comment 6

[Mathew Hodson]

Guidance from Microsoft:

https://msdn.microsoft.com/en-us/library/windows/desktop/ff485842(v=vs.85).aspx

"A method might return other success codes, so always test for errors by using the SUCCEEDED or FAILED macro."

"If you need to differentiate between S_OK and S_FALSE in your code, you should test the value directly, but still use FAILED or SUCCEEDED to handle the remaining cases."

Comment 7

[Chris Peterson [:cpeterson]]

This lint check could also apply to xpcom's NS_SUCCEEDED/NS_FAILED and NS_OK macros because there are success nsresult values other than NS_OK.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8938687 [details] [diff] [review]
Add mach lint check for comparisons with S_OK instead of using SUCCEEDED()

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

I think I'm okay with this, provided that we do another onceover to pick up any legit S_OK/S_FALSE checks that have appeared since this was written.

Comment 9

[Chris Peterson [:cpeterson]]

I think I'm okay with this, provided that we do another onceover to pick up any legit S_OK/S_FALSE checks that have appeared since this was written.

I rebased my lint patch and audited the new warnings. None of them look like bugs.

I can submit patches to add ((extra parens)) to suppress these new warnings, but I'm starting to think this lint is not a good idea. Some of the new warnings point to code that would become a bug if someone replaced hr == S_OK with SUCCEEDED(hr)) to silence the warning without thinking through the code paths carefully. Also, the extra parens are ugly.