Closed Bug 1332202 Opened 3 years ago Closed 3 years ago

Crash in OOM | large | NS_ABORT_OOM | NS_EscapeURL

Categories

(Core :: General, defect, critical)

50 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: philipp, Assigned: erahm)

References

(Depends on 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
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.
https://hg.mozilla.org/mozilla-central/rev/b2239bbe0c45
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please nominate this for Aurora/Beta approval when you feel it's sufficiently baked :)
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 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)
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)
Depends on: 1353738
You need to log in before you can comment on or make changes to this bug.