1329171 - Crash in OOM | large | NS_ABORT_OOM | nsACString_internal::BeginWriting | nsDataHandler::ParseURI

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-b34e3e4f-3677-4b36-8ab9-55ae22161221.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp:606
1 	xul.dll 	nsACString_internal::BeginWriting() 	xpcom/string/nsTSubstring.h:149
2 	xul.dll 	nsDataHandler::ParseURI(nsCString&, nsCString&, nsCString*, bool&, nsCString*) 	netwerk/protocol/data/nsDataHandler.cpp:164
3 	xul.dll 	nsDataHandler::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) 	netwerk/protocol/data/nsDataHandler.cpp:82
4 	xul.dll 	mozilla::net::nsIOService::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) 	netwerk/base/nsIOService.cpp:652
5 	xul.dll 	NS_NewURI(nsIURI**, nsACString_internal const&, char const*, nsIURI*, nsIIOService*) 	netwerk/base/nsNetUtilInlines.h:115
6 	xul.dll 	NS_NewURI(nsIURI**, nsAString_internal const&, char const*, nsIURI*, nsIIOService*) 	netwerk/base/nsNetUtilInlines.h:126
7 	xul.dll 	`anonymous namespace'::CheckMemAvailable 	xpcom/base/AvailableMemoryTracker.cpp:115
8 	mozglue.dll 	pages_decommit 	memory/mozjemalloc/jemalloc.c:2049
9 	mozglue.dll 	huge_palloc 	memory/mozjemalloc/jemalloc.c:5147
10 	ntdll.dll 	NtFreeVirtualMemory 	
11 	mozglue.dll 	pages_unmap 	memory/mozjemalloc/jemalloc.c:2387
12 	mozglue.dll 	base_node_dealloc 	memory/mozjemalloc/jemalloc.c:2204
13 	mozglue.dll 	huge_dalloc 	memory/mozjemalloc/jemalloc.c:5290
14 	mozglue.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:83
15 	fastprox.dll 	CWbemClass::SetData(unsigned char*, int) 	
16 	xul.dll 	`anonymous namespace'::CSSParserImpl::ParseDeclaration 	layout/style/nsCSSParser.cpp:7345
17 		@0x83 	
18 	xul.dll 	`anonymous namespace'::CSSParserImpl::SetStyleSheet 	layout/style/nsCSSParser.cpp:1592
19 	fastprox.dll 	CWbemClass::CompactAll() 	
20 	xul.dll 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/glue/nsCOMPtr.cpp:44
21 	xul.dll 	mozilla::css::Loader::LoadInlineStyle(nsIContent*, nsAString_internal const&, unsigned int, nsAString_internal const&, nsAString_internal const&, mozilla::dom::Element*, nsICSSLoaderObserver*, bool*, bool*) 	layout/style/Loader.cpp:2042
22 	xul.dll 	nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, bool*, bool) 	dom/base/nsStyleLinkElement.cpp:426
23 	xul.dll 	nsStyleLinkElement::UpdateStyleSheet(nsICSSLoaderObserver*, bool*, bool*, bool) 	dom/base/nsStyleLinkElement.cpp:222
24 	xul.dll 	nsXMLContentSink::HandleEndElement(char16_t const*, bool) 	dom/xml/nsXMLContentSink.cpp:1058
25 	xul.dll 	doContent 	parser/expat/lib/xmlparse.c:2554

these out of memory crashes have been around for a while - mainly on windows & 32bit versions of the browser.
during the 51 beta cycle the frequency of the crashes has increased though: https://crash-stats.mozilla.com/signature/?release_channel=beta&signature=OOM%20%7C%20large%20%7C%20NS_ABORT_OOM%20%7C%20nsACString_internal%3A%3ABeginWriting%20%7C%20nsDataHandler%3A%3AParseURI&date=%3E%3D2016-09-01T00%3A00%3A00.000Z#graphs

Comment 1

[(Away)]

nsDataHandler::ParseURI is memory-unfriendly on two counts:
- As these are content-controlled data: URIs, that BeginWriting should be fallible, but we can do even better:
- The function makes a writable copy of the string in order to do some manipulation on the "header" (not sure if that's the right technical term) of the URI. Since the header is small and the body can be large, maybe the function could make a copy only up to |comma|?

Nick, any Uptime folks interested in taking this?

Comment 2

[Loic]

I got this crash after switching between Flash video player and HTML5 player on Twitch.tv.

Comment 3

[Loic]

https://crash-stats.mozilla.com/report/index/6480618f-859e-4971-9670-fd2f32170108

Comment 4

[Nicholas Nethercote [inactive]]

I've worked on nsDataHandler::ParseURI() before (bug 1262359) so I will take this.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add some more cases to the data URI unit test

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Rewrite nsDataHandler::ParseURI

The new version has the following improvements.

- If the non-data part is empty, the spec isn't duplicated at all for parsing;
  otherwise, only the non-data part gets copied. This avoids a potential OOM
  case when the data part is large.

- The base64 parsing is moved later. It now only occurs once we've determined
  that the non-data part is non-empty.

- It now uses |sizeof| consistently to get the length of string literals.
  Previously it was a mix of |sizeof|, strlen() and hard-coded lengths.

- There's now no need to undo null-insertion once parsing is finished, because
  the parsing is done on a local copy of the non-data part.

Comment 7

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d99698a3046f0b97719972534ab7b3819137042

Comment 8

[Jason Duell]

Comment on attachment 8824864 [details] [diff] [review]
(part 2) - Rewrite nsDataHandler::ParseURI

Valentin, I'm guessing you're the best person to review these patches.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8824864 [details] [diff] [review]
(part 2) - Rewrite nsDataHandler::ParseURI

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

::: netwerk/protocol/data/nsDataHandler.cpp
@@ +170,5 @@
>  
>      // First, find the start of the data
> +    const char* roComma = strchr(roBuffer, ',');
> +    const char* roHash = strchr(roBuffer, '#');
> +    if (!roComma || (roHash && roHash < roComma))

nit: add curly braces

@@ +183,5 @@
>      } else {
> +        // Make a copy of the non-data part so we can null out parts of it as
> +        // we go. This copy will be a small number of chars, in contrast to the
> +        // data which may be large.
> +        char* buffer = PL_strndup(roBuffer, roComma - roBuffer);

nit: could we use nsAutoCString instead of manual memory management?

Comment 10

[Nicholas Nethercote [inactive]]

> > +        char* buffer = PL_strndup(roBuffer, roComma - roBuffer);
> 
> nit: could we use nsAutoCString instead of manual memory management?

We need to operate on the char* buffer because there is no nsCString equivalent to PL_strcasestr(), as far as I know. It might be possible to transfer the buffer to an nsAutoCString, but IMHO it's clearer to not mix char* and nsCString operations for a single string.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3422f6703d05f36aafa56007486438689f70c8c8
Bug 1329171 (part 1) - Add some more cases to the data URI unit test. r=jduell.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0283ede511c4f279f8ecbce6f59c52515a794a
Bug 1329171 (part 2) - Rewrite nsDataHandler::ParseURI. r=jduell.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3422f6703d05
https://hg.mozilla.org/mozilla-central/rev/3a0283ede511

Comment 13

[[:philipp]]

is this something that we should try to uplift to aurora or beta even?
(as mentioned in comment #0 this signature is getting more common with firefox 51)

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8824864 [details] [diff] [review]
(part 2) - Rewrite nsDataHandler::ParseURI

Approval Request Comment
[Feature/Bug causing the regression]: N/A. Just an OOM in data URI parsing.

[User impact if declined]: more OOM crashes.

[Is this code covered by automated tests?]: Yes, and the other patch extended the testing a bit.

[Has the fix been verified in Nightly?]: In terms of the effect on OOM crashes: no. The OOM doesn't occur on Nightly -- possibly due to the small population and different hardware profile than Beta/Release -- so we can't look at crash stats to see an effect.

[Needs manual test from QE? If yes, steps to reproduce]: No. Reproducing is difficult anyway.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It's a minor rewrite of a single function, so is very contained.

[String changes made/needed]: None.

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8824863 [details] [diff] [review]
(part 1) - Add some more cases to the data URI unit test

This is just some extra tests for data URI parsing. It should be uplifted with the other patch.

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to [:philipp] from comment #13)
> is this something that we should try to uplift to aurora or beta even?

I've requested Aurora uplift. At this late stage in the cycle I don't think the crash frequency warrants Beta uplift.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8824864 [details] [diff] [review]
(part 2) - Rewrite nsDataHandler::ParseURI

avoid big memory allocation when parsing data: URIs, aurora52+

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8824863 [details] [diff] [review]
(part 1) - Add some more cases to the data URI unit test

test update, aurora52+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/777b047dc304
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3034da75775