Closed Bug 1270310 Opened 3 years ago Closed 3 years ago

Crash in OOM | large | NS_ABORT_OOM | nsAString_internal::BeginWriting

Categories

(Core :: XPCOM, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-4d86ff57-32c5-4bee-9a3b-7b52c2160428.
=============================================================

At least for this particular bug it looks like we should convert to a fallible |BeginWriting| in |nsLinebreakConverter::ConvertStringLineBreaks| [1].

The bigger question is why we're even reallocing the string in |BeginWriting| in this situation, as judging by the call to |GetValue| [2] it looks like it should not be shared.
So a few things we can probably do:

#1 - Don't bother trying line conversion if the element is a single line text control
#2 - Make the |BeginWriting| call fallible in the line conversion code
#3 - Probably make the Assign call in GetValue fallible as well
#4 - Also we could consider improving the line break conversion code so an in situ replace works without allocating on Windows. Right now the in situ logic bails if the src and dst line break formats aren't one character [1], that could be loosened to len(src) >= len(dst)

[1] https://dxr.mozilla.org/mozilla-central/rev/369a5ee3a2880a4a98df3a00bf3db8d8f36b181b/xpcom/io/nsLinebreakConverter.cpp#416-418
#5 - ConvertStringLineBreaks does a |nsString::Assign| [1] without providing the string length even though we know it. This leads to an unnecessary strlen call.

[1] https://dxr.mozilla.org/mozilla-central/rev/369a5ee3a2880a4a98df3a00bf3db8d8f36b181b/xpcom/io/nsLinebreakConverter.cpp#479
ConvertStringLineBreaks calls ConvertUnicharLineBreaksInSitu which uses
fallible allocation. We should make the potential allocation in |BeginWriting|
fallible as well and handle the failure. This also updates the callers to
|ConvertStringLineBreaks| to handle the error properly in release builds.
Attachment #8749441 - Flags: review?(nfroyd)
Attachment #8749441 - Flags: review?(bugs)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
The length of the new buffer being assigned in ConvertStringLineBreaks has
already been calculated, so we can pass that in.
Attachment #8749442 - Flags: review?(nfroyd)
This makes the string assignment fallible and also adds checks for the return
value from GetValueInternal and GetValue.
Attachment #8749443 - Flags: review?(bugs)
For single line text controls we shouldn't need to convert line breaks.
Attachment #8749444 - Flags: review?(bugs)
Attachment #8749441 - Flags: review?(nfroyd) → review+
Comment on attachment 8749442 [details] [diff] [review]
Part 2: Pass buffer length to Assign call in ConvertStringLineBreaks

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

::: xpcom/io/nsLinebreakConverter.cpp
@@ +477,5 @@
>      return rv;
>    }
>  
>    if (stringBuf != aIoString.get()) {
> +    aIoString.Adopt(stringBuf, newLen - 1);

This code and the need to +1 and -1 all over the place is just dumb.  And zero mention of it in the interfaces, too.
Attachment #8749442 - Flags: review?(nfroyd) → review+
It would probably be good to add nsAString_internal::BeginWriting to the skip list, as you really want to know who is calling it.
Comment on attachment 8749441 [details] [diff] [review]
Part 1: Make allocation in ConvertStringLineBreaks fallible

I have a bit long review queue, perhaps peterv can help here.
Attachment #8749441 - Flags: review?(bugs) → review?(peterv)
Attachment #8749443 - Flags: review?(bugs) → review?(peterv)
Attachment #8749444 - Flags: review?(bugs) → review?(peterv)
Attachment #8749441 - Flags: review?(peterv) → review+
Comment on attachment 8749443 [details] [diff] [review]
Part 3: Make string assignment in HTMLInputElement::GetValueInternal fallible

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

You need to change the SetterThrows for the value attribute in dom/webidl/HTMLInputElement.webidl to Throws, since now the getter throws too. r+ with that.
Attachment #8749443 - Flags: review?(peterv) → review+
Attachment #8749444 - Flags: review?(peterv) → review+
Peter, this updates the webidl file and adds a corresponding GetValue method.
Can you double-check I did the right thing implementing the new GetValue
method?
Attachment #8755052 - Flags: review?(peterv)
Attachment #8749443 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8755052 - Flags: review?(peterv) → review+
Comment on attachment 8749441 [details] [diff] [review]
Part 1: Make allocation in ConvertStringLineBreaks fallible

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

::: dom/html/HTMLInputElement.cpp
@@ -5766,5 @@
>               value,
>               nsLinebreakConverter::eLinebreakPlatform,
>               nsLinebreakConverter::eLinebreakContent);
> -      NS_ASSERTION(NS_SUCCEEDED(rv), "Converting linebreaks failed!");
> -      inputState->SetValue(value);

Hmm, removing the SetValue is probably wrong here. Sorry for missing that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cf86d4874699644b4c2a97ff2b2a3e9fe0e1cc
Bug 1270310 - Part 1: Make allocation in ConvertStringLineBreaks fallible. r=froydnj, r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/af33a66c68116bf306a9607cb3c9ad76deaa9b0f
Bug 1270310 - Part 2: Pass buffer length to Assign call in ConvertStringLineBreaks. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/37037b0016f65988bef198c34fdd47158aa83eaa
Bug 1270310 - Part 3: Make string assignment in HTMLInputElement::GetValueInternal fallible. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a422e5afb098f711f8f0ebdd139b7f78dcb94c0f
Bug 1270310 - Part 4: Bypass line break conversion if element is single line. r=smaug
Flags: needinfo?(erahm)
Crash Signature: [@ OOM | large | NS_ABORT_OOM | nsAString_internal::BeginWriting] → [@ OOM | large | NS_ABORT_OOM | nsAString_internal::BeginWriting] [@ OOM | large | NS_ABORT_OOM | nsAString_internal::BeginWriting | mozilla::dom::HTMLInputElement::SaveState] [@ OOM | large | NS_ABORT_OOM | nsAString_internal::BeginWriting | nsLinebrea…
You need to log in before you can comment on or make changes to this bug.