Open
Bug 1426722
Opened 7 years ago
Updated 2 years ago
Audit explicit checks against S_OK and use SUCCEEDED() or FAILED() instead
Categories
(Core :: Widget: Win32, defect, P3)
Core
Widget: Win32
Tracking
()
ASSIGNED
People
(Reporter: Gijs, Assigned: cpeterson)
References
Details
Attachments
(1 file)
(deleted),
patch
|
bugzilla
:
feedback+
|
Details | Diff | Splinter Review |
(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?
Flags: needinfo?(jmathies)
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
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))`.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
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.
Attachment #8938687 -
Flags: feedback?(aklotz)
Assignee | ||
Updated•7 years ago
|
Summary: Audit explicit checks against S_OK / S_FALSE and use FAILED() and similar macros/helpers instead → Audit explicit checks against S_OK and use SUCCEEDED() or FAILED() instead
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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."
Updated•7 years ago
|
Flags: needinfo?(jmathies)
Assignee | ||
Comment 7•7 years ago
|
||
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.
Blocks: 1027713
status-firefox58:
--- → wontfix
Updated•7 years ago
|
Priority: -- → P3
Comment 8•5 years ago
|
||
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.
Attachment #8938687 -
Flags: feedback?(aklotz) → feedback+
Assignee | ||
Comment 9•5 years ago
|
||
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.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•