Make NS_EscapeURL more fallible

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 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

3 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...
Assignee

Comment 2

3 years ago
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
Assignee

Comment 3

3 years ago
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
Assignee

Updated

3 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee

Comment 7

3 years ago
Previously we avoided copying the input string if modification was not
necessary. This remimplements that behavior.
Attachment #8764792 - Flags: review?(mcmanus)
Assignee

Comment 9

3 years ago
Attachment #8764794 - Flags: review?(nfroyd)
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)
Assignee

Comment 12

3 years ago
Updated for test failures, latest run looks happy.
Attachment #8765059 - Flags: review?(valentin.gosu)
Assignee

Updated

3 years ago
Attachment #8764792 - Attachment is obsolete: true
Attachment #8764792 - Flags: review?(valentin.gosu)
Assignee

Comment 13

3 years ago
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)
Assignee

Updated

3 years ago
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+
Assignee

Comment 19

3 years ago
(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).
Assignee

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03371049b8602d22188c0bf733eb8407754af17
Bug 1249352 - Part 1: Remove unnecessary flat string in SetSpec. r=valentin

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa2d3b94b609f57252fa6a1cf9e179fbfeb5c75
Bug 1249352 - Part 2: Modify net_FilterURIString behavior to avoid unnecessary copying. r=valentin

https://hg.mozilla.org/integration/mozilla-inbound/rev/f06c76914e32dc08b43b50f55f62f413e525b392
Bug 1249352 - Part 3: Remove requirement to copy spec when calling NS_EscapeURL. r=valentin

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b305170f6498a666e49fbab097c42064b796bb2
Bug 1249352 - Part 4: Add a fallible NS_EscapeURL. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7e16969ae6bc05aa309fca6a9ee886aa216dba
Bug 1249352 - Part 5: Udpate SetScheme to use fallible NS_EscapeURL. r=valentin
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
Comment #22 suggests that it is fixed too
You need to log in before you can comment on or make changes to this bug.