1253059 - Large OOM in mozilla::dom::workers::WorkerPrivate::ReportError, mostly on Google Docs

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-102548e3-adc7-4453-a8bb-0e6482160302.
=============================================================

Stack Trace:
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp
1 	xul.dll 	AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&) 	xpcom/string/nsReadableUtils.cpp
2 	xul.dll 	NS_ConvertUTF8toUTF16::NS_ConvertUTF8toUTF16(char const*) 	xpcom/string/nsString.h
3 	xul.dll 	mozilla::dom::workers::WorkerPrivate::ReportError(JSContext*, char const*, JSErrorReport*) 	dom/workers/WorkerPrivate.cpp
4 	xul.dll 	mozilla::dom::AutoJSAPI::ReportException() 	dom/base/ScriptSettings.cpp
5 		@0x46cff227 	
6 	xul.dll 	js::ObjectGroupCompartment::makeGroup(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, unsigned int) 	js/src/vm/ObjectGroup.cpp
7 	xul.dll 	js::jit::GenerateNewObjectWithTemplateCode 	js/src/jit/BaselineIC.cpp
8 	xul.dll 	js::jit::GenerateNewObjectWithTemplateCode 	js/src/jit/BaselineIC.cpp


Top URLs:
18 	https://docs.google.com/spreadsheets/d/1XX5nsdi5nuJtUsCKpj_DzzB8bQtrGI2yL5lKaA2ijEI/edit#gid=958459105
14 	https://mail.google.com/mail/u/0/#inbox
9 	https://docs.google.com/spreadsheets/d/1JUAWbMyBp3SxF7poQ_aILrsDevDiFZqlMyX5W1z9SW0/edit?ts=569d9550#gid=87949966
7 	https://mail.google.com/mail/u/0/#starred/14a4c2166c949818
7 	about:blank
6 	https://docs.google.com/spreadsheets/d/1C7rCraouE0S-4NbUPLsojrXAWVmuxmNnrHun7-qLtRk/edit#gid=1729049395
6 	https://docs.google.com/spreadsheets/d/1XX5nsdi5nuJtUsCKpj_DzzB8bQtrGI2yL5lKaA2ijEI/edit#gid=407031948
5 	https://docs.google.com/spreadsheets/d/1n8Jb0gP0QmMOGYdnXYGRc0vk_Ri6_HQQBmkeLFee054/edit#gid=1331574074
5 	https://mail.google.com/mail/u/1/#inbox
5 	https://docs.google.com/spreadsheets/d/1_rI2ADI4bCgbwuET0r6f8tUaLCpCkp3xYHAl8CKA39k/edit#gid=0
4 	https://docs.google.com/spreadsheets/d/1ildBjgiotFIyiFfPhXJCfOscEG0aZgCeRUDMU16Mbms/edit?ts=56a358a8#gid=1630825027
4 	https://docs.google.com/spreadsheets/d/1mkoovVzkxay9NWN6o10CCKmy_16NG_x60bNKP_Nev7s/edit#gid=1407514676
4 	https://docs.google.com/spreadsheets/d/1-HW8GCjG0L7fgMctxgiYITPqnAIjbKsVYubtDMi_o5s/edit#gid=0
4 	https://docs.google.com/spreadsheets/d/1JmJ4C8RszBJ3y2gTp5nSEzljN8buFz2nh3XYXZDCt2o/edit#gid=1364362182
4 	https://docs.google.com/spreadsheets/d/1IiNullo4GIWPcl0jDOYIrYN0DHRt681KYhY54eZsrGk/edit?ts=56d49d32#gid=766538961
4 	https://docs.google.com/spreadsheets/d/1MKAxR7Gy_ohExD7AdfqLtYHRRIRnS3EBawRfY5nQCgE/edit#gid=357607522
4 	https://docs.google.com/spreadsheets/d/1UZV7buWU2ig2WgML8mmdXKIfqe6bViC4SeWGJFJvpNk/edit#gid=614353334
4 	https://docs.google.com/spreadsheets/d/1dd7oUbRtBSrv6NtQv1a4OevPd3oSWk88gOGRczJh7go/edit?invite=CPmV4-MO&pref=2&pli=1#gid=472135171
[a lot more with 3 or fewer hits over the last week, mostly docs.google.com]

This signature happens in 44.0.2 and earlier versions as well, but seems to be higher in volume in recent 45 betas and the RC.

This crash is pretty bad for people because they seem to crash repeatedly.

Many of the allocation sizes there are in the multi-megabyte area, for anything doing allocations that large, we really should resort to fallible allocation and catch the errors.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I agree, although I'm quite surprised this allocation can get to this size ...

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8726474 [details] [diff] [review]
Patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +5796,5 @@
>  
>    if (message.IsEmpty()) {
> +    nsDependentCString fallbackMessage(aFallbackMessage);
> +    if (!AppendUTF8toUTF16(fallbackMessage, message, mozilla::fallible)) {
> +      message = NS_LITERAL_STRING("*Out of memory*");

So, we are reporting an error message here. But we cannot allocate such string. Can we just ignore this error message instead? Otherwise we should try to localize it...
Also because this 'out of memory' message is not the reason why we are here: errorNumber will be probably 0 and we are going to have fireAtScope to true.
What about if we set errorNumber to JSMSG_OUT_OF_MEMORY?

Comment 4

[Robert Kaiser]

(In reply to Andrea Marchesini (:baku) from comment #3)
> So, we are reporting an error message here. But we cannot allocate such
> string.

Well, to be sure, the allocation that fails here is a "large" one, so at least more than 256K, often multiple megabytes, that string probably doesn't need that much.

Comment 5

[nemo]

Coworker encountered this crash repeatedly on a small simple form he was working on (2 rows of frozen header with merged cells, 15 lines of text content).

He triggered it with =ARRAYFORMULA(ROW(A:A)) as someone recommended online.

Which kept blowing up google spreadsheet causing errors and periodically crashing Firefox.  I don't know if it was due to the lack of bounds or the merged cells or both.

It did it fairly reproducibly anyway.
In his case:
=ARRAYFORMULA(row(A3:A30)-2)

Seemed to work a lot better. (in the first cell past the frozen header)

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Comment on attachment 8726474 [details] [diff] [review]
> Patch
> 
> Review of attachment 8726474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +5796,5 @@
> >  
> >    if (message.IsEmpty()) {
> > +    nsDependentCString fallbackMessage(aFallbackMessage);
> > +    if (!AppendUTF8toUTF16(fallbackMessage, message, mozilla::fallible)) {
> > +      message = NS_LITERAL_STRING("*Out of memory*");
> 
> So, we are reporting an error message here. But we cannot allocate such
> string. Can we just ignore this error message instead? Otherwise we should
> try to localize it...

Why would we localize it ...?

I could truncate the string instead, if you'd prefer that.  Maybe to 1KB?

Comment 7

[Andrea Marchesini [:baku]]

> I could truncate the string instead, if you'd prefer that.  Maybe to 1KB?

Yes, please.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8728823 [details] [diff] [review]
Patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +5799,5 @@
> +    if (!AppendUTF8toUTF16(fallbackMessage, message, mozilla::fallible)) {
> +      // Try again, with only a 1 KB string. Do this infallibly this time.
> +      // If the user doesn't have 1 KB to spare we're done anyways.
> +      nsDependentCString truncatedFallbackMessage(aFallbackMessage, 1024);
> +      AppendUTF8toUTF16(truncatedFallbackMessage, message);

In theory also this one could fail. What about (up to you here..):

if (!AppendUTF8toUTF16(truncateFallbackMessage, message, mozilla::fallible)) {
  return;
}

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

As I said in the comment, if we don't have 1KB to spare we should just crash.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0ba0a7671c

Comment 12

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> As I said in the comment, if we don't have 1KB to spare we should just crash.

Yes, allocation up to about 256K should be fine to be infallible and just crash (and we log them as "OOM | small" signature).

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff0ba0a7671c