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

RESOLVED FIXED in Firefox 54

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philipp, Assigned: froydnj)

Tracking

({crash, regression})

53 Branch
mozilla55
x86
Windows
crash, regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
[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
status-firefox55: ? → affected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking for 54/55.  Volume doesn't seem quite high enough for a 53 update.
status-firefox53: affected → wontfix
tracking-firefox54: ? → +
tracking-firefox55: ? → +
(Assignee)

Updated

a year ago
See Also: → bug 1362390
(Assignee)

Comment 3

a year ago
Created attachment 8864849 [details] [diff] [review]
part 1 - add a fallible CopyASCIItoUTF16 function

We already have all the machinery to expose a function like this, we
just need to write it.
Attachment #8864849 - Flags: review?(continuation)
(Assignee)

Comment 4

a year ago
Created attachment 8864850 [details] [diff] [review]
part 2 - make Base64Decode tolerant of allocation failures

We need to use a fallible CopyASCIItoUTF16 function, since we might not
have enough memory to perform the copy.
Attachment #8864850 - Flags: review?(continuation)
(Assignee)

Comment 5

a year ago
Created attachment 8864851 [details] [diff] [review]
part 3 - make Base64Decode even more tolerant of allocation failures

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+

Comment 7

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a219f72ba8dc
part 1 - add a fallible CopyASCIItoUTF16 function; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8835be784a0d
part 2 - make Base64Decode tolerant of allocation failures; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/989ceac2e515
part 3 - make Base64Decode even more tolerant of allocation failures; r=mccr8
(Assignee)

Updated

a year ago
Blocks: 1362449

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a219f72ba8dc
https://hg.mozilla.org/mozilla-central/rev/8835be784a0d
https://hg.mozilla.org/mozilla-central/rev/989ceac2e515
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 9

a year ago
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+
(Assignee)

Comment 13

a year ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Backed out for bustage.
> https://treeherder.mozilla.org/logviewer.html#?job_id=98097830&repo=mozilla-
> beta
> 
> https://hg.mozilla.org/releases/mozilla-beta/rev/e6d59d3fce9c

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.