Closed Bug 1272813 Opened 4 years ago Closed 4 years ago

NonVoidStringToJsval should fallibly create mutableCopy

Categories

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

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- affected
firefox48 --- affected
firefox49 --- fixed
firefox-esr45 --- affected

People

(Reporter: mccr8, Assigned: froydnj)

Details

(Keywords: crash, Whiteboard: btpp-backlog [tw-dom])

Crash Data

Attachments

(3 files, 1 obsolete file)

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.)
> 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?
Andrew, what do you want to do here?Are you interested/able to take this?
Flags: needinfo?(continuation)
Whiteboard: btpp-followup-2016-05-31
(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.
Flags: needinfo?(continuation)
Whiteboard: btpp-followup-2016-05-31 → btpp-backlog
Whiteboard: btpp-backlog → btpp-backlog [tw-dom]
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...
Assignee: nobody → nfroyd
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.
Avoiding large infallible allocations is always a good thing.
Attachment #8756847 - Flags: review?(peterv)
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.
Attachment #8756848 - Flags: review?(peterv)
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...
Attachment #8756849 - Flags: review?(peterv)
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 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
(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.
I think so too, so let's do it.  ;)
Now with more JS_ReportOutOfMemory.
Attachment #8757001 - Flags: review?(peterv)
Attachment #8756847 - Attachment is obsolete: true
Attachment #8756847 - Flags: review?(peterv)
Attachment #8757001 - Flags: review?(peterv) → review+
Attachment #8756848 - Flags: review?(peterv) → review+
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.
Attachment #8756849 - Flags: review?(peterv) → review+
Pushed by nfroyd@mozilla.com:
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
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.