Closed Bug 1377101 Opened 3 years ago Closed 3 years ago

Bug 1376951 Let's use CheckedUint32 more broadly in XHR code

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file)

No description provided.
Attachment #8882151 - Flags: review?(michael)
Comment on attachment 8882151 [details] [diff] [review]
checked_xhr.patch

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

Patch looks good, but I don't see what this has to do with CheckedUint32. It looks to me like this code is simply improving our error handling in XHR code.

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ -316,5 @@
>    mBlobSet = nullptr;
>    mResultArrayBuffer = nullptr;
>    mArrayBufferBuilder.reset();
>    mResultJSON.setUndefined();
> -  mDataAvailable = 0;

I appreciate that we have so many unused variables ^.^

@@ +1722,5 @@
>      if (xmlHttpRequest->mArrayBufferBuilder.capacity() == 0)
>        xmlHttpRequest->mArrayBufferBuilder.setCapacity(std::max(count, XML_HTTP_REQUEST_ARRAYBUFFER_MIN_SIZE));
>  
> +    if (NS_WARN_IF(!xmlHttpRequest->mArrayBufferBuilder.append(
> +                      reinterpret_cast<const uint8_t*>(fromRawSegment), count,

nit: Can you move count down a line? It's kinda hiding off to the right here.
Attachment #8882151 - Flags: review?(michael) → review+
> Patch looks good, but I don't see what this has to do with CheckedUint32. It
> looks to me like this code is simply improving our error handling in XHR
> code.

Right, because at the beginning I was using CheckedInt for mDataAvailable. But then I realize that we don't use this variable at all.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04091fe27f7
Add some return value checks in XHR, r=mystor
https://hg.mozilla.org/mozilla-central/rev/c04091fe27f7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.