Crash in OOM | large | NS_ABORT_OOM | NS_EscapeURL

RESOLVED FIXED in Firefox 52

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: erahm)

Tracking

(Depends on: 1 bug, {crash, regression})

50 Branch
mozilla54
crash, regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
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?
Flags: needinfo?(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.
Assignee: nobody → erahm
Flags: needinfo?(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
Created attachment 8829705 [details] [diff] [review]
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
Attachment #8829705 - Flags: review?(nfroyd)
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(...);|?
Attachment #8829705 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2239bbe0c45b84159ea8f5359de473a07a4b3dc
Bug 1332202 - Convert most infallible NS_EscapeURL calls to fallible version. r=froydnj
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2239bbe0c45
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please nominate this for Aurora/Beta approval when you feel it's sufficiently baked :)
status-firefox51: affected → wontfix
Flags: needinfo?(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.
Flags: needinfo?(erahm)
Attachment #8829705 - Flags: approval-mozilla-beta?
Attachment #8829705 - Flags: approval-mozilla-aurora?
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.
Attachment #8829705 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/34591732740b
status-firefox53: affected → fixed
Comment on attachment 8829705 [details] [diff] [review]
Convert most infallible NS_EscapeURL calls to fallible version

crash fix, should be in 52.0b2
Attachment #8829705 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs rebasing for beta

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

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d6ff11588e1d
status-firefox52: affected → fixed
Flags: qe-verify-
What do these comments in nsStandardURL mean?

    // This is left fallible as this entire function is expected to be
    // infallible.
Flags: needinfo?(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.
Flags: needinfo?(erahm)
Mind fixing the comments?
Flags: needinfo?(erahm)
Flags: needinfo?(erahm)

Updated

2 years ago
Depends on: 1353738
You need to log in before you can comment on or make changes to this bug.