FetchEventRunnable::HandleBodyWithHeaders is not really used

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: catalinb, Assigned: catalinb)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

While investigating bug 1333182, I discovered that we no longer need to manually parse the content of upload streams to extract headers. After bug 1305162, nsMIMEInputStream stores request headers separately from the base stream and no longer sets the flag noting the presence of headers in the body.

The relevant wpt test for this is: fetch-event.https.html.
Comment on attachment 8834042 [details] [diff] [review]
Remove code handling MIME input streams with bodies that contain headers.

Sorry, but I don't see the code removal in this patch.  Did you forget to qref?
Attachment #8834042 - Flags: review?(bkelly)
Wrong patch, sorry about that.
Attachment #8834042 - Attachment is obsolete: true
Attachment #8834043 - Flags: review?(bkelly)
Comment on attachment 8834043 [details] [diff] [review]
Remove code handling MIME input streams with bodies that contain headers.

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

Looks good, but I think we should have a strong assert here to make sure it doesn't change again in the future.  Thanks!

::: dom/workers/ServiceWorkerPrivate.cpp
@@ -1460,5 @@
>      if (uploadChannel) {
>        MOZ_ASSERT(!mUploadStream);
> -      bool bodyHasHeaders = false;
> -      rv = uploadChannel->GetUploadStreamHasHeaders(&bodyHasHeaders);
> -      NS_ENSURE_SUCCESS(rv, rv);

Can you add a MOZ_DIAGNOSTIC_ASSERT() that verifies this returns false?  Something like:

#if defined(DEBUG) || !defined(RELEASE_OR_BETA)
      bool bodyHasHeaders = false;	
      rv = uploadChannel->GetUploadStreamHasHeaders(&bodyHasHeaders);	
      NS_ENSURE_SUCCESS(rv, rv);
      MOZ_DIAGNOSTIC_ASSERT(!bodyHasHeaders);
#endif
Attachment #8834043 - Flags: review?(bkelly) → review+
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae8504e5999
Remove code handling MIME input streams with bodies that contain headers. r=bkelly
I landed the patch removing our headers parsing in SW code. Although it is called when resending post data, it doesn't really do anything useful since the stream it gets doesn't have headers.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1333182#c10
See Also: → 1333182
https://hg.mozilla.org/mozilla-central/rev/5ae8504e5999
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Catalin, can you uplift to FF53 since bug 1305162 is fixed there as well?  Saw another crash today on FF53.0a2:

https://crash-stats.mozilla.com/report/index/856ca9ba-4c88-4c0d-a320-312b12170215
Flags: needinfo?(catalin.badea392)
Or maybe this is still another issue with the post on document refresh?  Sorry, I'm kind of confused where we are on all of this.
Right now, with this patch we've switched from broken and crashing to just broken. I've filed bug 1341301 and I'll request an uplift.
Flags: needinfo?(catalin.badea392)
Comment on attachment 8834043 [details] [diff] [review]
Remove code handling MIME input streams with bodies that contain headers.

Approval Request Comment
[Feature/Bug causing the regression]: Service Worker
[User impact if declined]: OOM crash when resending post data while using a registered sw that just resets the intercepted channel.
[Is this code covered by automated tests?]: No, there's bug 1341301.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: SW only
[String changes made/needed]: none

This patch removes a code path that would perform an unnecessary header parsing, which could trigger a OOM crash.
Attachment #8834043 - Flags: approval-mozilla-aurora?
Comment on attachment 8834043 [details] [diff] [review]
Remove code handling MIME input streams with bodies that contain headers.

Fix a potential OOM crash. Aurora53+.
Attachment #8834043 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.