Use ReadSegments in FetchDriver::OnDataAvailable even in the SRI case

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Would avoid some buffer copies, and arguably the code is simpler.
Created attachment 8873706 [details] [diff] [review]
Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable

This avoids some buffer copies and is arguably more readble.
Attachment #8873706 - Flags: review?(bkelly)
Comment on attachment 8873706 [details] [diff] [review]
Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable

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

Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +658,5 @@
>  };
>  
> +struct SRIVerifierAndOutputHolder {
> +  SRICheckDataVerifier* mVerifier;
> +  nsIOutputStream* mOutputStream;

Please use a constructor to initialize and delete the default constructor.  I just worry about someone using these uninitialized pointers in the future.

@@ +671,5 @@
> +                          uint32_t aOffset,
> +                          uint32_t aCount,
> +                          uint32_t* aCountWritten)
> +{
> +  auto holder = static_cast<SRIVerifierAndOutputHolder*>(aClosure);

Can you add a MOZ_DIAGNOSTIC_ASSERT() that holder and its contents are not nullptr?

@@ +680,5 @@
> +
> +  // The rest is just like NS_CopySegmentToStream.
> +  *aCountWritten = 0;
> +  while (aCount) {
> +    uint32_t n;

Please initialize this variable.

@@ +682,5 @@
> +  *aCountWritten = 0;
> +  while (aCount) {
> +    uint32_t n;
> +    rv = holder->mOutputStream->Write(aBuffer, aCount, &n);
> +    if (NS_FAILED(rv)) {

Do you not use NS_ENSURE_SUCCESS() here because you are avoiding the warning?  Or because it was like that in the old code?
Attachment #8873706 - Flags: review?(bkelly) → review+

Comment 3

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61599ed0f713
Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable.  r=bkelly
Comment hidden (mozreview-request)
Comment on attachment 8874709 [details]
Bug 1369619.  Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable.

Sorry, don't know what happened here when committing to mozreview
Attachment #8874709 - Attachment is obsolete: true
Attachment #8874709 - Flags: review?(bkelly)
> Please use a constructor to initialize and delete the default constructor.

Done. 

> Can you add a MOZ_DIAGNOSTIC_ASSERT() that holder and its contents are not nullptr?

Done.

> Please initialize this variable.

Done.

> Do you not use NS_ENSURE_SUCCESS() here because you are avoiding the warning?  Or because it was like that in the old code?

Honestly, because I copy/pasted NS_CopySegmentToStream.  But if I had to pick, avoiding the warning here makes sense, because NS_BASE_STREAM_WOULD_BLOCK is a totally valid failure return value here.

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61599ed0f713
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.