1337033 - FetchEventRunnable::HandleBodyWithHeaders is not really used

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

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 1

[Cătălin Badea (:catalinb)]

Attachment: Remove code handling MIME input streams with bodies that contain headers.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 3

[Cătălin Badea (:catalinb)]

Attachment: Remove code handling MIME input streams with bodies that contain headers.

Wrong patch, sorry about that.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae8504e5999
Remove code handling MIME input streams with bodies that contain headers. r=bkelly

Comment 6

[Cătălin Badea (:catalinb)]

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

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ae8504e5999

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Cătălin Badea (:catalinb)]

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 11

[Cătălin Badea (:catalinb)]

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 12

[Gerry Chang [:gchang]]

Comment on attachment 8834043 [details] [diff] [review]
Remove code handling MIME input streams with bodies that contain headers.

Fix a potential OOM crash. Aurora53+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6345d678d9c3