Closed Bug 1333182 Opened 4 years ago Closed 3 years ago

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

Categories

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

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bkelly, Assigned: catalinb)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

This bug was filed from the Socorro interface and is 
report bp-a8d3668b-2245-4fd8-802a-278db2170123.
=============================================================
This is due to serializing the upload stream into a single string.  It looks like we copy the string a few times as well.
Crash Signature: [@ OOM | large | NS_ABORT_OOM | libxul.so@0x9a3438 | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent] This is due to serializing the upload stream into a single string. It looks like we copy the string a few times as well. → [@ OOM | large | NS_ABORT_OOM | libxul.so@0x9a3438 | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent]
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.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Crash Signature: [@ OOM | large | NS_ABORT_OOM | libxul.so@0x9a3438 | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent] → [@ OOM | large | NS_ABORT_OOM | libxul.so@0x9a3438 | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign | mozilla::dom::workers::`anonymous namespace''::FetchEventRunnable::HandleBodyW…
Priority: -- → P3
Crash Signature: namespace''::FetchEventRunnable::HandleBodyWithHeaders ] → namespace''::FetchEventRunnable::HandleBodyWithHeaders ] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign | mozilla::dom::workers::`anonymous namespace''::FetchEventRunnable::Init ]
Crash Signature: namespace''::FetchEventRunnable::HandleBodyWithHeaders ] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign | mozilla::dom::workers::`anonymous namespace''::FetchEventRunnable::Init ] → namespace''::FetchEventRunnable::HandleBodyWithHeaders ] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign | mozilla::dom::workers::`anonymous namespace''::FetchEventRunnable::Init ] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign…
Crash Signature: nsACString_internal::Assign | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] → nsACString_internal::Assign | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ]
After bug 1305162, we no longer hit this codepath, at least in our web platform tests.
Assignee: nobody → catalin.badea392
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
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
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?
Status: NEW → ASSIGNED
Flags: needinfo?(catalin.badea392)
Crash Signature: nsACString_internal::Assign | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] → nsACString_internal::Assign | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] [@ OOM | large | NS_ABORT_OOM | libxul.so@0x99d693 | mozilla::dom:…
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.
Flags: needinfo?(catalin.badea392)
OK, this happens if you submit a form, then refresh the page and click OK in the "Resend data.. " dialog.
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.
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.
Flags: needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
See Also: → 1337033
Crash Signature: mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] → mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ] [@ OOM | large | NS_ABORT_OOM | nsACString_internal::Assign | nsACString_internal::Assign | mozilla::dom::workers::ServiceWorkerPrivate::SendFetchEvent ]
Bug 1337033 removed the code path that was causing this crash.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
See Also: → 1341301
You need to log in before you can comment on or make changes to this bug.