Closed Bug 562403 Opened 10 years ago Closed 7 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file)

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;
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?
I find this approach disturbing as it means that error values can either be 40x or 32bits.
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #444373 - Flags: feedback?(cbiesinger)
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.
Attachment #444373 - Flags: feedback?(cbiesinger)
(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)
(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
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.
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.
Looks like Aryeh just fixed this in bug 782594:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb941410b40

Resolving as FIXED by that bug.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Depends on: 782594
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.