1332202 - Crash in OOM | large | NS_ABORT_OOM | NS_EscapeURL

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-f147e003-e5e1-4710-9a6b-fec5a2170119.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp:606
1 	xul.dll 	NS_EscapeURL(char const*, int, unsigned int, nsACString_internal&) 	xpcom/io/nsEscape.cpp:479
2 	xul.dll 	NS_EscapeURL(nsACString_internal const&, unsigned int, nsACString_internal&) 	obj-firefox/dist/include/nsEscape.h:158
3 	xul.dll 	mozilla::net::nsSimpleURI::GetAsciiSpec(nsACString_internal&) 	netwerk/base/nsSimpleURI.cpp:544
4 	xul.dll 	nsDataChannel::OpenContentStream(bool, nsIInputStream**, nsIChannel**) 	netwerk/protocol/data/nsDataChannel.cpp:29
5 	xul.dll 	nsBaseChannel::BeginPumpingData() 	netwerk/base/nsBaseChannel.cpp:235
6 	xul.dll 	nsBaseChannel::AsyncOpen(nsIStreamListener*, nsISupports*) 	netwerk/base/nsBaseChannel.cpp:676
7 	xul.dll 	nsBaseChannel::AsyncOpen2(nsIStreamListener*) 	netwerk/base/nsBaseChannel.cpp:704

crashes with this signature are regressing in volume since firefox 50, mostly on windows and android platforms. the user comments almost all talk about uploading an image file at the time of the crash to various different sites: 
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=OOM%20|%20large%20|%20NS_ABORT_OOM%20|%20NS_EscapeURL&date=%3E%3D2016-07-19T08%3A16%3A36.000Z&date=%3C2017-01-19T08%3A16%3A36.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#comments

Comment 1

[(Away)]

It looks like a large fraction of NS_EscapeURL callers use nsresult error handling, and could pretty easily be switched to the fallible version. Any reason not to go through and do a blanket conversion?

Comment 2

[Eric Rahm [:erahm]]

(In reply to David Major [:dmajor] from comment #1)
> It looks like a large fraction of NS_EscapeURL callers use nsresult error
> handling, and could pretty easily be switched to the fallible version. Any
> reason not to go through and do a blanket conversion?

Seems like a job for MOZ_MUST_USE. I'll take a look at how hard it would be to convert all callers.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #2)
> (In reply to David Major [:dmajor] from comment #1)
> > It looks like a large fraction of NS_EscapeURL callers use nsresult error
> > handling, and could pretty easily be switched to the fallible version. Any
> > reason not to go through and do a blanket conversion?
> 
> Seems like a job for MOZ_MUST_USE. I'll take a look at how hard it would be
> to convert all callers.

I guess MOZ_MUST_USE won't help here, the signature is a bit different. A quick search indicates 17 users of the non-fallible version [1].

[1] http://searchfox.org/mozilla-central/search?q=symbol:_Z12NS_EscapeURLRK10nsACStringjRS_&redirect=false

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3921bf38faa3

Comment 5

[Eric Rahm [:erahm]]

Attachment: Convert most infallible NS_EscapeURL calls to fallible version

This makes most users of |NS_EscapeURL| use the fallible version. A few are
left infallible as it seems like the entire function is assumed to be
infallible.

MozReview-Commit-ID: Cy1L5jQwjO1

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8829705 [details] [diff] [review]
Convert most infallible NS_EscapeURL calls to fallible version

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

I think this all makes sense.

::: netwerk/base/nsSimpleURI.cpp
@@ +660,5 @@
>      nsAutoCString buf;
>      nsresult rv = GetSpec(buf);
>      if (NS_FAILED(rv)) return rv;
> +    rv = NS_EscapeURL(buf, esc_OnlyNonASCII|esc_AlwaysCopy, result, fallible);
> +    return rv;

Maybe just |return NS_EscapeURL(...);|?

Comment 7

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2239bbe0c45b84159ea8f5359de473a07a4b3dc
Bug 1332202 - Convert most infallible NS_EscapeURL calls to fallible version. r=froydnj

Comment 8

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8829705 [details] [diff] [review]
> Convert most infallible NS_EscapeURL calls to fallible version
> 
> Review of attachment 8829705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this all makes sense.
> 
> ::: netwerk/base/nsSimpleURI.cpp
> @@ +660,5 @@
> >      nsAutoCString buf;
> >      nsresult rv = GetSpec(buf);
> >      if (NS_FAILED(rv)) return rv;
> > +    rv = NS_EscapeURL(buf, esc_OnlyNonASCII|esc_AlwaysCopy, result, fallible);
> > +    return rv;
> 
> Maybe just |return NS_EscapeURL(...);|?

Updated before landing.

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta approval when you feel it's sufficiently baked :)

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8829705 [details] [diff] [review]
Convert most infallible NS_EscapeURL calls to fallible version

Approval Request Comment
[Feature/Bug causing the regression]:

N/A, latent issues.

[User impact if declined]:

OOM crashes.

[Is this code covered by automated tests?]:

In URI code which is thoroughly exercised.

[Has the fix been verified in Nightly?]:

Nightly seems okay, I don't think we'll know until it hits beta.

[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 risk.

[Why is the change risky/not risky?]:

Just makes URI related allocations fallible, very simple changes.

[String changes made/needed]:

None.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8829705 [details] [diff] [review]
Convert most infallible NS_EscapeURL calls to fallible version

Crash fix, let's take it for aurora 53 and see how that affects things. We can consider it for beta uplift on Monday for beta 2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34591732740b

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8829705 [details] [diff] [review]
Convert most infallible NS_EscapeURL calls to fallible version

crash fix, should be in 52.0b2

Comment 15

[Carsten Book [:Tomcat]]

needs rebasing for beta

warning: conflicts while merging dom/storage/DOMStorageObserver.cpp! (edit, then use 'hg resolve --mark')

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6ff11588e1d

Comment 17

[Boris Zbarsky [:bzbarsky]]

What do these comments in nsStandardURL mean?

    // This is left fallible as this entire function is expected to be
    // infallible.

Comment 18

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> What do these comments in nsStandardURL mean?
> 
>     // This is left fallible as this entire function is expected to be
>     // infallible.

Sorry that's a typo, should say "This is left infallible..." 

It's meant to convey that nsStandardURL::GetASCIISpec is expected to be infallible (it always returns NS_OK or crashes), switching over to an infallible version of NS_EscapeURL would just confuse things.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Mind fixing the comments?

Comment 20

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0588456829814fac5e00bafed5f9ced886afdf6
Bug 1332202 - Followup to fix comment to say infallible. DONTBUILD r=me

Comment 21

[Carsten Book [:Tomcat]]

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