783523 - Fix platform-specific nsresult misuse

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

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

Comment 2

[(no longer active)]

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.

Comment 3

[Trevor Saunders (:tbsaunde)]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 657226 [details] [diff] [review]
Patch

r=me for the printing parts

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=06d3fb315a2a

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

New try without broken patches from an unrelated bug:

https://tbpl.mozilla.org/?tree=Try&rev=f08fd8c4ffe9

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/844b142d8111

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/844b142d8111