Make NS_EscapeURL more fallible

RESOLVED FIXED in Firefox 49

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: erahm)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 fixed, firefox-esr45 wontfix, firefox50 fixed)

Details

(crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The number 81 crash on release (and the second most common OOM | large) is [@ OOM | large | NS_ABORT_OOM | T_EscapeURL<T> ].

In this particularly egregious example, OOM allocation size is around 130MB:
https://crash-stats.mozilla.com/report/index/624525e4-7195-4d10-a34e-7bd792160211

The crashes I looked at were all in this loop, on the append(), so maybe the append should be made fallible:

    // Flush the temp buffer if it doesnt't have room for another encoded char.
    if (tempBufferPos >= mozilla::ArrayLength(tempBuffer) - ENCODE_MAX_LEN) {
      NS_ASSERTION(writing, "should be writing");
      aResult.Append(tempBuffer, tempBufferPos);
      tempBufferPos = 0;
    }

There's at least some code set up to handle failure.
I dunno, the comment for NS_EscapeURL makes it sound like the return value is pretty useless:

 * @return true if aResult was written to (i.e. at least one character was
 *              escaped or esc_AlwaysCopy was requested), false otherwise.

In addition, most callers don't check for failure (not that failure would mean what one would think in the first place), and I'm not sure if updating all those callsites (and transitively, and so on) to expect failure is an easy prospect.

What in the world are people escaping that causes such allocations?  Big data: URLs?  The stack from comment 0 makes it look like that...
There's all sorts of fun things going on here:
- First we convert a nsString to a nsCString (so roughly 1.5X the memory now) [1]
- Then we do filter in SetSpec [2], it's not clear to me if that copies. I think it does (so roughly 2X the memory now)
- Then we do an escape in SetSpec [3], that makes a copy no matter what (so roughly 2.5X the memory now)

Lets go a little further, I'm reasonably sure this is a nsSimpleURI we're dealing with and the load will be sync, if so:
- At this point the temp strings should be out of scope, so we're back to 1.5X the memory
- we build another copy when loading the image [4,5] (so roughly 2X the memory now)
- and so on, we probably end up base64 decoding to yet another string, then imglib will probably copy that into a legit buffer, I'd guess we're at 3.5X at this point

Anyhow all that said, this is a failure building a URI, and that certainly is expected to fail. So propagating failure here would make sense.

[1] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsNetUtilInlines.h#126
[2] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsSimpleURI.cpp#203
[3] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsSimpleURI.cpp#209
[4] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/image/imgLoader.cpp#2066
[5] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsSimpleURI.cpp#168
Things we can improve on:

#1 - We don't need a flat string in SetSpec [1]
#2 - We can revert |net_FilterURIString| back to it's old behavior of not copying if not necessary [2]
#3 - We can remove the |esc_AlwaysCopy| flag from the call to |NS_EscapeUrl| [3]
#4 - We can add a fallible version of NS_EscapeURL [4]. This might need an alternate name so as to avoid rewriting all the other callers (we have to change the return type).

[1] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsSimpleURI.cpp#199
[2] https://hg.mozilla.org/mozilla-central/diff/a10683e0ed00/netwerk/base/nsURLHelper.cpp#l573
[3] https://dxr.mozilla.org/mozilla-central/rev/b9f4f38063951cd5a8b249911aea61869f40fd1f/netwerk/base/nsSimpleURI.cpp#209
[4] https://dxr.mozilla.org/mozilla-central/rev/c9edfe35619f69f7785776ebd19a3140684024dc/xpcom/io/nsEscape.h#156
Created attachment 8764791 [details] [diff] [review]
Part 1: Remove unnecessary flat string in SetSpec
Attachment #8764791 - Flags: review?(mcmanus)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Created attachment 8764792 [details] [diff] [review]
Part 2: Modify net_FilterURIString behavior to avoid unnecessary copying

Previously we avoided copying the input string if modification was not
necessary. This remimplements that behavior.
Attachment #8764792 - Flags: review?(mcmanus)
Created attachment 8764793 [details] [diff] [review]
Part 3: Remove requirement to copy spec when calling NS_EscapeURL
Attachment #8764793 - Flags: review?(mcmanus)
Created attachment 8764794 [details] [diff] [review]
Part 4: Add a fallible NS_EscapeURL
Attachment #8764794 - Flags: review?(nfroyd)
Created attachment 8764795 [details] [diff] [review]
Part 5: Udpate SetScheme to use fallible NS_EscapeURL
Attachment #8764795 - Flags: review?(mcmanus)
Attachment #8764791 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8764792 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8764793 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8764795 - Flags: review?(mcmanus) → review?(valentin.gosu)
Created attachment 8765059 [details] [diff] [review]
Part 2: Modify net_FilterURIString behavior to avoid unnecessary copying

Updated for test failures, latest run looks happy.
Attachment #8765059 - Flags: review?(valentin.gosu)
Attachment #8764792 - Attachment is obsolete: true
Attachment #8764792 - Flags: review?(valentin.gosu)
Created attachment 8765060 [details] [diff] [review]
Part 4: Add a fallible NS_EscapeURL

Updated for windows build failure in the new test due to using \u0123 notation
in a string. Switched to \x instead.
Attachment #8765060 - Flags: review?(nfroyd)
Attachment #8764794 - Attachment is obsolete: true
Attachment #8764794 - Flags: review?(nfroyd)
Comment on attachment 8765059 [details] [diff] [review]
Part 2: Modify net_FilterURIString behavior to avoid unnecessary copying

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

Great improvement. Thanks!
Attachment #8765059 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8764791 [details] [diff] [review]
Part 1: Remove unnecessary flat string in SetSpec

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

I think the flat string used to be necessary when net_FilterURIString took a char* arg, instead of an nsACString, and things would go badly when SetSpec got passed a substring. This should be fine now.
Attachment #8764791 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8764793 [details] [diff] [review]
Part 3: Remove requirement to copy spec when calling NS_EscapeURL

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

r=valentin
Attachment #8764793 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8764795 [details] [diff] [review]
Part 5: Udpate SetScheme to use fallible NS_EscapeURL

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

r=valentin

Thanks!
Attachment #8764795 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8765060 [details] [diff] [review]
Part 4: Add a fallible NS_EscapeURL

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

Thank you for adding tests...we need a lot more, but I guess we can start somewhere.

::: xpcom/io/nsEscape.cpp
@@ +372,2 @@
>  T_EscapeURL(const typename T::char_type* aPart, size_t aPartLen,
> +            uint32_t aFlags, T& aResult, bool& aAppended)

Given that we're modifying this function, some documentation would be most appreciated.  It's not obvious to me what |aAppended| is for; perhaps it should be renamed |aDidEscape|?
Attachment #8765060 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #18)
> Comment on attachment 8765060 [details] [diff] [review]
> Part 4: Add a fallible NS_EscapeURL
> 
> Review of attachment 8765060 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for adding tests...we need a lot more, but I guess we can start
> somewhere.

I'll file a mentored follow-up.

> 
> ::: xpcom/io/nsEscape.cpp
> @@ +372,2 @@
> >  T_EscapeURL(const typename T::char_type* aPart, size_t aPartLen,
> > +            uint32_t aFlags, T& aResult, bool& aAppended)
> 
> Given that we're modifying this function, some documentation would be most
> appreciated.  It's not obvious to me what |aAppended| is for; perhaps it
> should be renamed |aDidEscape|?

Added a method doc, changed the param to |aDidAppend| as sometimes we append even if we don't escape (detailed in the added docs).
Crash volume for signature 'OOM | large | NS_ABORT_OOM | T_EscapeURL<T>':
 - beta (version 48): 1052 crashes from 2016-06-06.
 - esr  (version 45): 1764 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          0          0          0          0          0
 - beta           180        180        131        157        130        156         59
 - release       1312       1369       1245       1227       1159       1096        356
 - esr            211        209        174        211        163        203        169

Affected platform: Windows
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
(Reporter)

Updated

2 years ago
status-firefox47: affected → wontfix
status-firefox48: affected → wontfix
status-firefox-esr45: affected → wontfix
Comment #22 suggests that it is fixed too
status-firefox49: --- → fixed
You need to log in before you can comment on or make changes to this bug.