1333182 - Crash in OOM | large | NS_ABORT_OOM | libxul.so@0x9a3438 | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-a8d3668b-2245-4fd8-802a-278db2170123.
=============================================================

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This is due to serializing the upload stream into a single string.  It looks like we copy the string a few times as well.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I think the correct fix here is to create a "header removing" input stream type.  That can simply read the start of the stream into a reasonably sized buffer, remove the headers, and maintain the remainder as part of its state.  We certainly don't have to read the entire stream into memory to remove a few bytes at the start.

This should get fixed as part of our overall "make streaming better" in service workers effort.  I may not have time to work on it immediately.

Comment 3

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

After bug 1305162, we no longer hit this codepath, at least in our web platform tests.

Comment 4

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

As long as the input stream QIs to nsIMIMEInputStream, we will not hit this code, due to the changes in bug bug 1305162. According to the comment at [1] this always the case. As such, I'm inclined to close this bug as fixed. Ben, what do you think?

[1] http://searchfox.org/mozilla-central/rev/f5077ad52f8b90183e73038869f6140f0afbf427/dom/workers/ServiceWorkerPrivate.cpp#1605

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Ok.  Its unfortunate we can't fix it in FF52, though.  Bug 1305162 looks too large to uplift to beta.  Although I guess we could ask the author about uplift.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Although, this crash was in FF53.0a2:

https://crash-stats.mozilla.com/report/index/2579689a-c814-4fa0-8da6-945fe2170206

Build id is: 20170131084239

Does this suggest its still happening?

Comment 7

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

Looks like it. I'm a bit confused on what's going on, I can't figure out what type of request would trigger it.

Comment 8

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

OK, this happens if you submit a form, then refresh the page and click OK in the "Resend data.. " dialog.

Comment 9

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

So it looks like when we resend post data we get a stream that doesn't contain the headers anymore, even though the flag is set. Is this a bug?

This is the content of the upload stream we get in HandleBodyWith Headers after and before bug 1305162.
https://pastebin.mozilla.org/8977818

At the moment, HandleBodyWithHeaders just removes a leading delimiter - which just seems wrong.

Comment 10

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

After further investigating this, I believe registering a service worker breaks resending form POST requests (form submit, followed by page refresh) because we will fail to send the Content-Type and Content-Length headers.

This is a rough sequence of events that happen when you submit a form and then refresh the page:
Initial form submit:
1. HttpBaseChannel::SetUploadStream(mimeInputStream);
Because we got a nsIMIMEInputStream we don't set the hasHeaders flag.
http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/netwerk/protocol/http/HttpBaseChannel.cpp#759

2. HttpBaseChannel::ExplicitSetUploadStream(aStreamHeaders=false)
Here we copy the header info into the channel:
http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/netwerk/protocol/http/HttpBaseChannel.cpp#936

3. Next, we replace the upload stream with a nsStorageInputStream in SWM::DispatchFetchEvent when we call channel->EnsureUploadStreamIsCloneable. This is where we lose the header info.

4. Store the uploadStream in a nsSHEntry, but we will use the newly created nsStorageInputStream.

Refresh and click OK on the re-submit dialog:
1. HttpBaseChannel::SetUploadStream(storageInputStream); // from history
   Now we'll set the hasHeaders flag even though it doesn't.
2. We will eventually call into SWP::HandleBodyWithHeaders, which will mangle the data a bit more by removing a leading delimiter (at least for Content-Type: multipart/form-data;).

Ben, why do we need to copy the stream to a nsStorageInputStream? I think we need to use something that will preserve the headers as well.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cătălin Badea (:catalinb) from comment #10)
> Ben, why do we need to copy the stream to a nsStorageInputStream? I think we
> need to use something that will preserve the headers as well.

It was to make sure we had a stream we could serialize across IPC.  We might not need it any more if uploads are using PSendStream.

Comment 12

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

Bug 1337033 removed the code path that was causing this crash.