Closed Bug 783523 Opened 9 years ago Closed 9 years ago

Fix platform-specific nsresult misuse


(Core :: General, defect)

Not set





(Reporter: ayg, Assigned: ayg)




(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:

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.

Attachment #657226 - Flags: review?(ehsan)
Comment on attachment 657226 [details] [diff] [review]

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]

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

> 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]

r=me for the printing parts
Attachment #657226 - Flags: review?(bugs) → review+
New try without broken patches from an unrelated bug:
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.