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
Created attachment 172181 [details] Reduction (instant crash!)
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.
Created attachment 172749 [details] Patch
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?
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
Created attachment 173798 [details] Patch
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.
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.
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?
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.
(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.
Created attachment 173834 [details] Patch
(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
Comment on attachment 173834 [details] Patch ok
Comment on attachment 173834 [details] Patch Clearing flags on attachment: 173834 Committed r134377: <http://trac.webkit.org/changeset/134377>
All reviewed patches have been landed. Closing bug.
Reverted r134377 for reason: Caused a mysterious Android Clang build failure, needs investigation before landing again. Committed r134506: <http://trac.webkit.org/changeset/134506>
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
Was it fixed by https://bugs.webkit.org/show_bug.cgi?id=102108 ?
Doh! Can you guys re-land the patch?
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.
Sorry, Dimitri. Thank you, Kent-san. I will re-land the patch. Best regards, Takashi Sakamoto
Created attachment 174046 [details] Patch for landing
Comment on attachment 174046 [details] Patch for landing Clearing flags on attachment: 174046 Committed r134584: <http://trac.webkit.org/changeset/134584>