1249352 - Make NS_EscapeURL more fallible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

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

Comment 2

[Eric Rahm [:erahm]]

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

Comment 3

[Eric Rahm [:erahm]]

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

Comment 4

[Eric Rahm [:erahm]]

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

Comment 5

[Eric Rahm [:erahm]]

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

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove unnecessary flat string in SetSpec

Comment 7

[Eric Rahm [:erahm]]

Attachment: 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.

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 3: Remove requirement to copy spec when calling NS_EscapeURL

Comment 9

[Eric Rahm [:erahm]]

Attachment: Part 4: Add a fallible NS_EscapeURL

Comment 10

[Eric Rahm [:erahm]]

Attachment: Part 5: Udpate SetScheme to use fallible NS_EscapeURL

Comment 11

[Eric Rahm [:erahm]]

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

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 2: Modify net_FilterURIString behavior to avoid unnecessary copying

Updated for test failures, latest run looks happy.

Comment 13

[Eric Rahm [:erahm]]

Attachment: 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.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

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!

Comment 15

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

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

Comment 17

[Valentin Gosu [:valentin] (he/him)]

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!

Comment 18

[Nathan Froyd [:froydnj]]

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|?

Comment 19

[Eric Rahm [:erahm]]

(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).

Comment 20

[Eric Rahm [:erahm]]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c03371049b86
https://hg.mozilla.org/mozilla-central/rev/1fa2d3b94b60
https://hg.mozilla.org/mozilla-central/rev/f06c76914e32
https://hg.mozilla.org/mozilla-central/rev/2b305170f649
https://hg.mozilla.org/mozilla-central/rev/2d7e16969ae6

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

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 23

[Sylvestre Ledru [:Sylvestre]]

Comment #22 suggests that it is fixed too