Closed Bug 1362194 Opened 3 years ago Closed 3 years ago

Crash in OOM | large | NS_ABORT_OOM | AppendASCIItoUTF16 | CopyASCIItoUTF16 | mozilla::Base64Decode


(Core :: XPCOM, defect, critical)

53 Branch
Not set



Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed


(Reporter: philipp, Assigned: froydnj)



(Keywords: crash, regression)

Crash Data


(3 files)

This bug was filed from the Socorro interface and is 
report bp-029173b0-5249-4a04-bc5c-6489d0170426.
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp:598
1 	xul.dll 	AppendASCIItoUTF16(nsACString_internal const&, nsAString_internal&) 	xpcom/string/nsReadableUtils.cpp:179
2 	xul.dll 	CopyASCIItoUTF16(nsACString_internal const&, nsAString_internal&) 	xpcom/string/nsReadableUtils.cpp:92
3 	xul.dll 	mozilla::Base64Decode(nsAString_internal const&, nsAString_internal&) 	xpcom/io/Base64.cpp:472
4 	xul.dll 	nsContentUtils::Atob(nsAString_internal const&, nsAString_internal&) 	dom/base/nsContentUtils.cpp:853
5 	xul.dll 	mozilla::dom::WindowBinding::atob 	obj-firefox/dom/bindings/WindowBinding.cpp:14908
6 	xul.dll 	mozilla::dom::WindowBinding::genericMethod 	obj-firefox/dom/bindings/WindowBinding.cpp:15658
7 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:448
8 	xul.dll 	InternalCall 	js/src/vm/Interpreter.cpp:493
9 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:2990
10 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:394
11 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:466
12 	xul.dll 	js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp:512
13 	xul.dll 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp:2887
14 	xul.dll 	JS_InitializePropertiesFromCompatibleNativeObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) 	js/src/jsobj.cpp:1336

this oom crash signature from 32bit instances on windows is regressing in volume since firefox 53.0.
Component: Untriaged → XPCOM
[Tracking Requested - why for this release]: Spiking crash, might as well take care of it.

This is a failure to handle OOM properly in Base64Decode; I'll take care of fixing that.
Assignee: nobody → nfroyd
tracking for 54/55.  Volume doesn't seem quite high enough for a 53 update.
See Also: → 1362390
We already have all the machinery to expose a function like this, we
just need to write it.
Attachment #8864849 - Flags: review?(continuation)
We need to use a fallible CopyASCIItoUTF16 function, since we might not
have enough memory to perform the copy.
Attachment #8864850 - Flags: review?(continuation)
The lossy conversion to ASCII here can also fail; we should handle that
as well.  Rewriting the code to use MakeScopeExit also avoids tangled
logic and/or duplicating calls to ensure the destination string is
truncated on failure.
Attachment #8864851 - Flags: review?(continuation)
Attachment #8864849 - Flags: review?(continuation) → review+
Attachment #8864850 - Flags: review?(continuation) → review+
Comment on attachment 8864851 [details] [diff] [review]
part 3 - make Base64Decode even more tolerant of allocation failures

Review of attachment 8864851 [details] [diff] [review]:

::: xpcom/io/Base64.cpp
@@ +464,5 @@
>  nsresult
>  Base64Decode(const nsAString& aBase64, nsAString& aBinary)
>  {
> +  auto truncater = mozilla::MakeScopeExit([&]() { aBinary.Truncate(); });

nit: truncator, maybe?

@@ +467,5 @@
>  {
> +  auto truncater = mozilla::MakeScopeExit([&]() { aBinary.Truncate(); });
> +
> +  // XXX We should really consider decoding directly from the string, rather
> +  // than making a separate copy here.

Maybe file a bug for this and reference it here?
Attachment #8864851 - Flags: review?(continuation) → review+
Pushed by
part 1 - add a fallible CopyASCIItoUTF16 function; r=mccr8
part 2 - make Base64Decode tolerant of allocation failures; r=mccr8
part 3 - make Base64Decode even more tolerant of allocation failures; r=mccr8
Blocks: 1362449
Comment on attachment 8864849 [details] [diff] [review]
part 1 - add a fallible CopyASCIItoUTF16 function

This request is for all the patches in this bug.  These patches may just move the OOM crash around, but they are a start to fixing the problem.

Approval Request Comment
[Feature/Bug causing the regression]: Ancient.
[User impact if declined]: OOM crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[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?]: We are now failing on OOM on a common codepath, no other functionality changes.
[String changes made/needed]: None.
Attachment #8864849 - Flags: approval-mozilla-beta?
Comment on attachment 8864849 [details] [diff] [review]
part 1 - add a fallible CopyASCIItoUTF16 function

Let's take it in Beta54 and see how it goes. Beta54+. Should be in 54 beta 7.
Attachment #8864849 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8864850 - Flags: approval-mozilla-beta+
Attachment #8864851 - Flags: approval-mozilla-beta+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Backed out for bustage.
> beta

Ugh, OK.  The compilation error was hidden on inbound because another commit in the same push (bug 1362390) fixed the error.  Want a beta-specific patch, or just uplift the other bug, too (which would also address a few crashes)?
Flags: needinfo?(nfroyd)
Let's start with requesting approval of the other bug and do a branch patch if that gets denied.
(In reply to Nathan Froyd [:froydnj] from comment #9)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: No.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Nathan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.