Closed Bug 1396290 Opened 2 years ago Closed 2 years ago

Crash in OOM | large | NS_ABORT_OOM | nsACString::Assign

Categories

(Core :: DOM: File, defect, critical)

55 Branch
x86
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: philipp, Assigned: baku)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-24f886a0-c84f-4435-a74a-3cc5a0170902.
=============================================================
Crashing Thread (27), Name: IPDL Background
Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp:610
1 	xul.dll 	nsACString::Assign(nsACString const&) 	xpcom/string/nsTSubstring.cpp:421
2 	xul.dll 	nsStringInputStream::Clone(nsIInputStream**) 	xpcom/io/nsStringStream.cpp:387
3 	xul.dll 	mozilla::BasePrincipal::GetOriginSuffix(nsACString&) 	caps/BasePrincipal.cpp:300
4 	xul.dll 	nsMultiplexInputStream::Clone(nsIInputStream**) 	xpcom/io/nsMultiplexInputStream.cpp:1034
5 	xul.dll 	nsMultiplexInputStream::Clone(nsIInputStream**) 	xpcom/io/nsMultiplexInputStream.cpp:1034
6 	xul.dll 	NS_CloneInputStream(nsIInputStream*, nsIInputStream**, nsIInputStream**) 	xpcom/io/nsStreamUtils.cpp:923
7 	xul.dll 	mozilla::dom::IPCBlobInputStreamStorage::GetStream(nsID const&, nsIInputStream**) 	dom/file/ipc/IPCBlobInputStreamStorage.cpp:151
8 	xul.dll 	mozilla::dom::IPCBlobInputStreamParent::HasValidStream() 	dom/file/ipc/IPCBlobInputStreamParent.cpp:164
9 	xul.dll 	mozilla::ipc::BackgroundParentImpl::RecvPIPCBlobInputStreamConstructor(mozilla::ipc::PIPCBlobInputStreamParent*, nsID const&, unsigned __int64 const&) 	ipc/glue/BackgroundParentImpl.cpp:336
10 	xul.dll 	mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PBackgroundParent.cpp:2272
11 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp:2092
12 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message&&) 	ipc/glue/MessageChannel.cpp:2018
13 	xul.dll 	mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) 	ipc/glue/MessageChannel.cpp:1887
14 	xul.dll 	mozilla::ipc::MessageChannel::MessageTask::Run() 	ipc/glue/MessageChannel.cpp:1920
15 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
16 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:480
17 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:369
18 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:319
19 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:299
20 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:506
21 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
22 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
23 	ucrtbase.dll 	thread_start<unsigned int (__stdcall*)(void*)> 	
24 	kernel32.dll 	BaseThreadInitThunk 	
25 	mozglue.dll 	patched_BaseThreadInitThunk 	mozglue/build/WindowsDllBlocklist.cpp:815
26 	ntdll.dll 	__RtlUserThreadStart 	
27 	ntdll.dll 	_RtlUserThreadStart

these OOM crashes are starting with firefox 55 (related to bug 1340710?) on 32bit versions on windows but the reports get more frequent during the 56.0b cycle. last week this has been the #20 top crash on beta accounting for 0.6% of browser crashes.
(In reply to [:philipp] from comment #0)
> these OOM crashes are starting with firefox 55 (related to bug 1340710?) on
> 32bit versions on windows but the reports get more frequent during the 56.0b
> cycle. last week this has been the #20 top crash on beta accounting for 0.6%
> of browser crashes.

Unlikely. If anything, those changes probably make OOM less likely. And the origin strings in question are small enough that they're unlikely to show up in a significant number of OOM crashes.

In any case, the GetOriginSuffix() call in that stack is a red herring. The crash actually happens when copying the nsStringInputStream contents. This is more likely related to bug 1353629.
Blocks: 1353629
No longer blocks: 1340710
Component: Security: CAPS → DOM: File
baku, does anything stand out here? This is not a huge top crash in beta, but it does seem to have gotten worse in 56 beta.
Flags: needinfo?(amarchesini)
Attached patch string.patchSplinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8904872 - Flags: review?(nfroyd)
Comment on attachment 8904872 [details] [diff] [review]
string.patch

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

This is great, will probably help fix other crashes, too.  I don't know where to put the comment below; deleting it altogether is an option.  WDYT?

::: xpcom/io/nsStringStream.cpp
@@ +385,5 @@
>  {
> +  RefPtr<nsStringInputStream> ref = new nsStringInputStream();
> +
> +  // Use Assign() here because we don't want the life of the clone to be
> +  // dependent on the life of the original stream.

This comment seems a little out of place here.  Maybe it should cite the `Assign` call in SetData instead, or maybe SetData should have an explicit comment about `Assign`?
Attachment #8904872 - Flags: review?(nfroyd) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dada8a78fd1
nsStringStream should propagate the OOM error of SetData() when cloned, r=froydnj
https://hg.mozilla.org/mozilla-central/rev/6dada8a78fd1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This grafts cleanly to Beta. Please nominate it for approval when you're comfortable doing so.
Flags: needinfo?(amarchesini)
Comment on attachment 8904872 [details] [diff] [review]
string.patch

Approval Request Comment
[Feature/Bug causing the regression]: nsStringInputStream
[User impact if declined]:  OOM not correctly handled.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: hard to reproduce.
[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?]: low.
[Why is the change risky/not risky?]: It's just an error propagation of the return value of string::Assign()
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8904872 - Flags: approval-mozilla-beta?
Comment on attachment 8904872 [details] [diff] [review]
string.patch

fix an OOM. Beta56+.
Attachment #8904872 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.