Closed Bug 783523 Opened 7 years ago Closed 7 years ago

Fix platform-specific nsresult misuse

Categories

(Core :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file)

Up to now I've been compiling on localhost.  Now I did a bunch of try pushes and found a handful of non-Linux-i386 misuses that break other platforms when nsresult is an enum.  I'll submit a patch shortly, once I isolate all the Windows bustage.  I already have all non-Windows platforms building successfully on try.
Attached patch PatchSplinter Review
Almost all Windows code that confuses HRESULT with nsresult.  In case you don't know: HRESULT is the Windows API error code type that nsresult is based on.  It's structured in exactly the same way, in particular with the top bit used for success vs. failure.  FAILED() is the same as NS_FAILED().  Some of the common values are here:

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

In particular, S_OK == NS_OK == 0, and E_FAIL == NS_ERROR_FAILURE == 0x80004005.

In nsAccessNodeWrap.cpp, I use a static_cast from HRESULT to nsresult to avoid changing behavior.  As the comment indicates, this is wrong in one case -- the called method can return E_NOINTERFACE, which doesn't have the same numeric value as any existing nsresult.  Should I get module peer review on this?

In nsPrintProgress.cpp I cast bool to nsresult.  Maybe this needs module peer review.  The method is virtual and implemented in a million different places, so I don't know what effect changing behavior here would actually have.  Most of the implementations of the method don't actually use that parameter (aStatus), and the ones that do all treat it as an nsresult as far as I can tell, but a number of callers pass a boolean instead.

Try: https://tbpl.mozilla.org/?tree=Try&rev=544fb418994a
Attachment #657226 - Flags: review?(ehsan)
Comment on attachment 657226 [details] [diff] [review]
Patch

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

Looks good, but those two pieces that you mentioned need peer review.
Attachment #657226 - Flags: review?(ehsan)
Attachment #657226 - Flags: review?(dbolter)
Attachment #657226 - Flags: review?(bugs)
Attachment #657226 - Flags: review+
Comment on attachment 657226 [details] [diff] [review]
Patch

>-nsresult
>+HRESULT
> TextLeafAccessibleWrap::GetCharacterExtents(int32_t aStartOffset,
>                                             int32_t aEndOffset,
>                                             int32_t* aX,
>                                             int32_t* aY,
>                                             int32_t* aWidth,
>                                             int32_t* aHeight)
> {

you missed the NS_ENSURE_TRUE(frame, NS_EROR_FAILURE);
in this method making it E_FAIL would of course be fine.

although btw you could just remove the method and inline it at the one use.

> NS_IMETHODIMP
> nsAccessNodeWrap::QueryNativeInterface(REFIID aIID, void** aInstancePtr)
> {
>-  return QueryInterface(aIID, aInstancePtr);
>+  // XXX Wrong for E_NOINTERFACE
>+  return static_cast<nsresult>(QueryInterface(aIID, aInstancePtr));

So long as you don't plan to change the negative nsresults being failure and positive being success I suspect that's probably fine until we remove the method all together.
Attachment #657226 - Flags: review?(dbolter) → review+
Comment on attachment 657226 [details] [diff] [review]
Patch

r=me for the printing parts
Attachment #657226 - Flags: review?(bugs) → review+
New try without broken patches from an unrelated bug:

https://tbpl.mozilla.org/?tree=Try&rev=f08fd8c4ffe9
https://hg.mozilla.org/mozilla-central/rev/844b142d8111
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.