Bug 101133 – Crash when replacing parts of text inputs with content: url(...)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Elliott Sprehn]

When you replace a part of a text input with an image using content: url(...) we crash in WebCore::RenderTextControlSingleLine::layout()

    #0 0x7f3d63b22655 in WebCore::RenderTextControlSingleLine::layout() third_party/WebKit/Source/WebCore/rendering/RenderTextControlSingleLine.cpp:111
    #1 0x7f3d62f51f30 in WebCore::RenderObject::layoutIfNeeded() third_party/WebKit/Source/WebCore/rendering/RenderObject.h:662
    #2 0x7f3d630979d3 in WebCore::RenderBlock::layoutPositionedObjects(bool) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2682
    #3 0x7f3d6308bc34 in WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1588
    #4 0x7f3d63086cdd in WebCore::RenderBlock::layout() third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1383
    #5 0x7f3d62f51f30 in WebCore::RenderObject::layoutIfNeeded() third_party/WebKit/Source/WebCore/rendering/RenderObject.h:662
    #6 0x7f3d630979d3 in WebCore::RenderBlock::layoutPositionedObjects(bool) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2682
    #7 0x7f3d6308bc34 in WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1588
    #8 0x7f3d63086cdd in WebCore::RenderBlock::layout() third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1383


This is enough to cause it:

<style>
input::-webkit-textfield-decoration-container {
    content: url("");
}
</style>
<input type=number>

This is because of the hack in RenderObject::createObject where we swap out the correct renderer for an element with a RenderImage if content is specified and only has a url(...).

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&exact_package=chromium&q=RenderObject.cpp&type=cs&l=144

[Elliott Sprehn]

Created attachment 172181 [details]
Reduction (instant crash!)

[Elliott Sprehn]

I think the trivial fix for this is to override the content property in these cases:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/TextFieldInputType.cpp&exact_package=chromium&q=textfield-decoration-container&type=cs&l=248

we should do

m_container->setInlineStyleProperty(CSSPropertyContent, "none", false);

as I can't fathom why you'd ever actually intend to swap out the entire inside of a text input with a content image.

[Takashi Sakamoto]

Created attachment 172749 [details]
Patch

[Elliott Sprehn]

Comment on attachment 172749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172749&action=review

> Source/WebCore/css/html.css:439
> +    content: none !important;

Do !important rules in author style sheets not override ones in the UA sheet?

> LayoutTests/fast/dom/HTMLInputElement/input-set-content-url-crash.html:5
> +    content: url("");

What happens if you put an !important here?

[Eric Seidel (no email)]

Comment on attachment 172749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172749&action=review

I think we need some sort of ASSERT or other guard in the C++ code to make sure we don't ever hit this again.  Right now if we removed that line we would start having this crash again. I suspect that's true of many lines in our UA file, but it still seems worthy of a C++ ASSERT to validate our understanding of the code's limitations.

>> Source/WebCore/css/html.css:439
>> +    content: none !important;
> 
> Do !important rules in author style sheets not override ones in the UA sheet?

UA sheets win:
http://www.w3.org/TR/CSS2/cascade.html#cascading-order

[Takashi Sakamoto]

Created attachment 173798 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 172749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172749&action=review

Thank you for reviewing.

I added ASSERT to TextFieldInputType::attach() to confirm that container has no content data.

Best regards,
Takashi Sakamoto

>> LayoutTests/fast/dom/HTMLInputElement/input-set-content-url-crash.html:5
>> +    content: url("");
> 
> What happens if you put an !important here?

I agree that I should add !important here.
Done.

[Kent Tamura]

Comment on attachment 173798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173798&action=review

> Source/WebCore/ChangeLog:11
> +        Test: fast/dom/HTMLInputElement/input-set-content-url-crash.html

Would you put the test as fast/forms/number/number-content-url-crash.html please?
Also, this bug should happen with <input type=search>. We had better have the same test for type=search.

[Takashi Sakamoto]

Comment on attachment 173798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173798&action=review

>> Source/WebCore/ChangeLog:11
>> +        Test: fast/dom/HTMLInputElement/input-set-content-url-crash.html
> 
> Would you put the test as fast/forms/number/number-content-url-crash.html please?
> Also, this bug should happen with <input type=search>. We had better have the same test for type=search.

So... I know the following input types inherit from class TextFieldInputType:
- TextInputType
- URLInputType
- EmailInputType
- SearchInputType
- PasswordInputType
- TelephoneInputType
- NumberInputType
- BaseDateAndTimeInputType

We have to add the same test for above all types?

[Kent Tamura]

Comment on attachment 173798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173798&action=review

>>> Source/WebCore/ChangeLog:11
>>> +        Test: fast/dom/HTMLInputElement/input-set-content-url-crash.html
>> 
>> Would you put the test as fast/forms/number/number-content-url-crash.html please?
>> Also, this bug should happen with <input type=search>. We had better have the same test for type=search.
> 
> So... I know the following input types inherit from class TextFieldInputType:
> - TextInputType
> - URLInputType
> - EmailInputType
> - SearchInputType
> - PasswordInputType
> - TelephoneInputType
> - NumberInputType
> - BaseDateAndTimeInputType
> 
> We have to add the same test for above all types?

No. -webkit-textfield-decoration-container is used only in type=number and type=search.

[Takashi Sakamoto]

(In reply to comment #10)
> (From update of attachment 173798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173798&action=review
> 
> >>> Source/WebCore/ChangeLog:11
> >>> +        Test: fast/dom/HTMLInputElement/input-set-content-url-crash.html
> >> 
> >> Would you put the test as fast/forms/number/number-content-url-crash.html please?
> >> Also, this bug should happen with <input type=search>. We had better have the same test for type=search.
> > 
> > So... I know the following input types inherit from class TextFieldInputType:
> > - TextInputType
> > - URLInputType
> > - EmailInputType
> > - SearchInputType
> > - PasswordInputType
> > - TelephoneInputType
> > - NumberInputType
> > - BaseDateAndTimeInputType
> > 
> > We have to add the same test for above all types?
> 
> No. -webkit-textfield-decoration-container is used only in type=number and type=search.

I see.

[Takashi Sakamoto]

Created attachment 173834 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 173798 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173798&action=review
> > 
> > >>> Source/WebCore/ChangeLog:11
> > >>> +        Test: fast/dom/HTMLInputElement/input-set-content-url-crash.html
> > >> 
> > >> Would you put the test as fast/forms/number/number-content-url-crash.html please?
> > >> Also, this bug should happen with <input type=search>. We had better have the same test for type=search.
> > > 
> > > So... I know the following input types inherit from class TextFieldInputType:
> > > - TextInputType
> > > - URLInputType
> > > - EmailInputType
> > > - SearchInputType
> > > - PasswordInputType
> > > - TelephoneInputType
> > > - NumberInputType
> > > - BaseDateAndTimeInputType
> > > 
> > > We have to add the same test for above all types?
> > 
> > No. -webkit-textfield-decoration-container is used only in type=number and type=search.

Thanks.

> I see.

I added number-content-url-crash.html and search-content-url-crash.html instead of input-set-content-url-crash.html.

Best regards,
Takashi Sakamoto

[Kent Tamura]

Comment on attachment 173834 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 173834 [details]
Patch

Clearing flags on attachment: 173834

Committed r134377: <http://trac.webkit.org/changeset/134377>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Reverted r134377 for reason:

Caused a mysterious Android Clang build failure, needs investigation before landing again.

Committed r134506: <http://trac.webkit.org/changeset/134506>

[Dimitri Glazkov (Google)]

This caused a weird breakage upstream:

http://build.chromium.org/p/chromium.linux/builders/Android%20Clang%20Builder%20%28dbg%29/builds/1281/steps/compile/logs/stdio

[Kent Tamura]

Was it fixed by https://bugs.webkit.org/show_bug.cgi?id=102108 ?

[Dimitri Glazkov (Google)]

Doh!
Can you guys re-land the patch?

[Eric Seidel (no email)]

There is no real need for these two tests to be separate. :)  But that also shouldn't stand in the way of this patch (re-)landing.

[Takashi Sakamoto]

Sorry, Dimitri.
Thank you, Kent-san.

I will re-land the patch.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 174046 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174046 [details]
Patch for landing

Clearing flags on attachment: 174046

Committed r134584: <http://trac.webkit.org/changeset/134584>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.