800603 - Don't bother rv-checking *and* outparam-null-checking, when calling imgIRequest::GetImage(). (just need to rv-check)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

I just noticed this place where we (I think) needlessly null-check the result of imgIRequest::GetImage():

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?rev=53edf545ee94#4028

It looks like we only have one imgIRequest impl -- imgRequestProxy -- and its GetImage() method will fail (return NS_ERROR_FAILURE) if its returned image is going to be null.[2]  So when it succeeds, we're guaranteed that its outparam is non-null.  So it's redundant to error-check the "rv" value *and* the returned pointer.

I'm filing this as a general bug because I'm pretty sure this happens elsewhere, too.

[1] https://mxr.mozilla.org/mozilla-central/source/image/public/imgIRequest.idl?rev=f4157e8c4107#23
[2] https://mxr.mozilla.org/mozilla-central/source/image/src/imgRequestProxy.cpp?rev=68dd2bcc51ee&mark=446-447#437

Comment 1

[Daniel Holbert [:dholbert]]

(This is all true of GetImagePrincipal(), too, actually.)

Comment 2

[Daniel Holbert [:dholbert]]

I did a quick MXR search to find instances of this, and they were all in /layout (though that's where I focused, so it's possible there are some elsewhere and I just missed them.)  Hence, filing under /layout.

(Also: There's a lot of other code that calls "request->GetImage(getter_AddRefs(img)))" and then *just* null-checks img, without checking whether GetImage() failed. That's technically un-kosher, but it's very prevalent and not a huge deal, so I'm basically not concerning myself with that in this bug.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix 1: check rv instead of outparam

Comment 4

[Daniel Holbert [:dholbert]]

(Note: This patch does make us check both the rv and the outparam *inside of assertions* in a few places, since perf doesn't matter there, and we might as well be extra-robust in what we're asserting.)

Comment 5

[Daniel Holbert [:dholbert]]

(also, this patch touches the same code as bug 799335 (that's how I ran across this issue) -- I'll wait for bug 799335 to land and fix the bitrot in this patch afterwards.)

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 670594 [details] [diff] [review]
fix 1: check rv instead of outparam

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

This seems somewhat ok. In the long term it would be better if GetImage() just returned the pointer instead of RV

Comment 7

[Daniel Holbert [:dholbert]]

Yup, agreed. (and in the meantime, this hopefully moves us towards being more consistent & less redundant)

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c2e416f76d6

I'll land this when the m-i is looking quieter/greener than it is right now.

Comment 8

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3de1ec1e38cf

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/3de1ec1e38cf