562403 - nsPrefetchNode::GetStatus warning: large integer implicitly truncated to unsigned type

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[timeless]

In member function ‘virtual nsresult nsPrefetchNode::GetStatus(PRUint16*)’:
nsPrefetchService.cpp:858: warning: large integer implicitly truncated to unsigned type

nsPrefetchNode::GetStatus(PRUint16 *aStatus)
            *aStatus = NS_ERROR_NOT_AVAILABLE;

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

I don't immediately see whether this is defined in any spec (HTML5 offline looks quite different), so I'm not sure what the best fix is, but maybe this should just become a PRUint32 like for XMLHttpRequest?

Comment 2

[timeless]

Attachment: approach from comment 1

I find this approach disturbing as it means that error values can either be 40x or 32bits.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 444373 [details] [diff] [review]
approach from comment 1

find a DOM peer for this patch, please.

hmm, I thought that XMLHttpRequest also returned nsresults in its GetStatus function, but upon reading the code, it seems that it doesn't, at least not anymore. so maybe that should be "return rv" instead.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
>             *aStatus = NS_ERROR_NOT_AVAILABLE;

Actually, *all three* of the aStatus assignments in nsPrefetchNode::GetStatus() are wrong, as noted in Bug 565500 comment 0:
> AFAICT, nsPrefetchNode::GetStatus is supposed to return one of the
> nsIDOMLoadStatus enumerated values in its PRUint16 outparam. (e.g.
> nsIDOMLoadStatus::REQUESTED)
> 
> However, right now it doesn't use any of those enumerated values, and instead
> it sets its outparam to one of the following:
>    - 0                                                   (line 841 & 861)
>    - NS_ERROR_NOT_AVAILABLE                              (line 857)
>    - PRUint16(httpStatus) where httpStatus is a 32-bit HTTP response (line 866)
(corrected the last line of the above-quoted chunk, per Bug 565500 comment 4)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Actually, *all three* of the aStatus assignments in nsPrefetchNode::GetStatus()
> are wrong, as noted in Bug 565500 comment 0:

Actually, I take that back -- it looks like at least in some circumstances, we expect nsIDOMLoadStatus to return a (16-bit version of a) HTTP response code -- here's one instance where we check a nsOfflineManifestItem against the values "404" and "410":
http://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp?mark=1349-1354#1349

Comment 7

[Daniel Holbert [:dholbert]]

Note that httpStatus (a PRUint32) ultimately traces back to a PRUint16 value, returned by nsHttpResponseHead::Status() (via nsHttpChannel::GetResponseStatus, in the main nsIHTTPChannel implementation).

So in nsPrefetchNode::GetStatus and nsOfflineCacheUpdateItem::GetStatus, the "*aStatus = PRUint16(httpStatus);" lines shouldn't overflow the maximum 16-bit unsigned int -- though it'd probably be nice to assert that.

Comment 8

[Josh Matthews [:jdm]]

I don't actually see where we would use this status value for prefetch nodes, since the only code that seems to care about nsIDOMLoadStatus is the offline manifest code, which strictly deals with nsOfflineManifestItem. However, I think the correct thing to do here is simply assign a status of 0 like the offline manifest item code does in the case of errors.

Comment 10

[Daniel Holbert [:dholbert]]

Looks like Aryeh just fixed this in bug 782594:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb941410b40

Resolving as FIXED by that bug.