Closed
Bug 562403
Opened 15 years ago
Closed 12 years ago
nsPrefetchNode::GetStatus warning: large integer implicitly truncated to unsigned type
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: timeless, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [build_warning])
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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.
Comment 3•15 years ago
|
||
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)
Comment 5•15 years ago
|
||
(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•15 years ago
|
||
(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
Updated•15 years ago
|
Whiteboard: [build_warning]
Comment 7•15 years ago
|
||
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.
Updated•13 years ago
|
Blocks: buildwarning
Comment 8•13 years ago
|
||
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•12 years ago
|
||
Looks like Aryeh just fixed this in bug 782594:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb941410b40
Resolving as FIXED by that bug.
Updated•12 years ago
|
Assignee: timeless → nobody
Assignee | ||
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•