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?

Wrong patch, sorry about that.

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

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

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

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.

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.

Comment on attachment 8834043 [details] [diff] [review] Remove code handling MIME input streams with bodies that contain headers. Fix a potential OOM crash. Aurora53+.