1272813 - NonVoidStringToJsval should fallibly create mutableCopy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-69aed055-22f2-4e36-8ef1-1f3b62160507.
=============================================================

This signature is not great (nsString::nsString should get added to the prefix list), but looking at a sample of 6 or so of these, almost all of them were in NonVoidStringToJsval, getting called from mozilla::dom::EventOrString::ToJSVal. These allocations are around 1MB. If we copied this string fallibly (which I'm guessing is coming from content somehow) then we could fail here instead of crashing.  (This is the #119 crasher on beta, across all processes, FWIW, and there are 178 in the last 7 days.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> which I'm guessing is coming from content somehow

This is calling the onerror handler on window, passing the first argument: the error message.  Content can pretty easily control it.  Testcase:

  onerror = function(str) {
    alert(str);
  }
  throw "Hellow there";

And we do end up copying here, because the code in JSEventHandler::HandleEvent sets up the string like so:

146       msgOrEvent.SetAsString().Rebind(errorMsg.Data(), errorMsg.Length());

which means we don't have a stringbuffer.  That's actually fairly annoying in this case, since I expect the caller _does_ have a stringbuffer in errorMsg....

Anyway, I have no problem with creating mutableCopy fallibly.  Do we have a way of doing that?

Comment 2

[Andrew Overholt [:overholt]]

Andrew, what do you want to do here?Are you interested/able to take this?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> Andrew, what do you want to do here? Are you interested/able to take this?

I don't have time to work on this. It also doesn't seem very common. But it would be a nice little win if somebody wants to fix an OOM | large.

Comment 4

[Nathan Froyd [:froydnj]]

I can do this...but do we want to fix JSEventHandler::HandleEvent at the same time?  Might as well try to take advantage of string sharing...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, we should.  The current setup dates back to when FakeString only supported being a dependent string.  Now that it can be a shareable buffer, we could just add a way to assign to it from an nsString which tries to share the buffer, if any, and falls back to rebinding as now otherwise (or I guess copying....).  We just have to be a bit careful about how we name it so people don't misuse it.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 1 - fallibly assign strings in {NonVoid,}StringToJsval

Avoiding large infallible allocations is always a good thing.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 2 - factor out a AssignFromStringBuffer method in FakeString

We're shortly going to have multiple ways to assign an nsStringBuffer
into FakeString, so we might as well put the common code somewhere
useful.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 3 - try to take advantage of string sharing in JSEventHandler::HandleEvent

The Rebind() call in HandleEvent was attempting to be clever by sharing
string data with the error event's message.  Unfortunately, we
eventually needed to pass the message out to JS, which required copying
the string for JS's purposes.  Fortunately, we can attempt to be even
more clever by noticing whether the error event's message is already
allocated as a string buffer and sharing that, rather than just the raw
data.  In the best case, the string buffer can be shared out to JS and
we avoid some needless copying.

Bikeshedding welcome on the name; I attempted to make it as obvious as bz
suggested in comment 5 what was going on...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8756847 [details] [diff] [review]
part 1 - fallibly assign strings in {NonVoid,}StringToJsval

Return false without setting a pending exception means an uncatchable exception.  Is that really what we want here?  Or do we want to JS_ReportOutOfMemory(cx)?

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8756849 [details] [diff] [review]
part 3 - try to take advantage of string sharing in JSEventHandler::HandleEvent

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

::: dom/bindings/BindingUtils.h
@@ +1901,5 @@
> +      Rebind(aString.Data(), aString.Length());
> +    } else {
> +      AssignFromStringBuffer(sharedBuffer.forget());
> +      mLength = aString.Length();
> +    }      

nit: trailing whitespace

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Return false without setting a pending exception means an uncatchable
> exception.  Is that really what we want here?  Or do we want to
> JS_ReportOutOfMemory(cx)?

I think we want JS_ReportOutOfMemory, but I don't feel qualified enough to argue strongly for that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think so too, so let's do it.  ;)

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 1 - fallibly assign strings in {NonVoid,}StringToJsval

Now with more JS_ReportOutOfMemory.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8756849 [details] [diff] [review]
part 3 - try to take advantage of string sharing in JSEventHandler::HandleEvent

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

I tried to think of a better name for ShareOrDependUpon, but nothing comes to mind. Oh well.
Please do document what it does though.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4d4bfb868b
part 1 - fallibly assign strings in {NonVoid,}StringToJsval; r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa493b763fb7
part 2 - factor out a AssignFromStringBuffer method in FakeString; r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c15fd63bd8
part 3 - try to take advantage of string sharing in JSEventHandler::HandleEvent; r=peterv

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a4d4bfb868b
https://hg.mozilla.org/mozilla-central/rev/fa493b763fb7
https://hg.mozilla.org/mozilla-central/rev/c3c15fd63bd8

Comment 17

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

Crash volume for signature 'OOM | large | NS_ABORT_OOM | nsString::nsString':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 1 crash from 2016-06-07.
 - beta    (version 48): 570 crashes from 2016-06-06.
 - release (version 47): 735 crashes from 2016-05-31.
 - esr     (version 45): 131 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          1          0          0          0          0
 - beta           134        116         30        248         30          1          2
 - release         42         62        126        144        157        141         42
 - esr              2          5         31         17         25         21         11

Affected platform: Windows