Summary: | Crash when replacing parts of text inputs with content: url(...) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Takashi Sakamoto <tasak> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, eric, inferno, jchaffraix, macpherson, menard, mifenton, mitz, tasak, tkent, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Elliott Sprehn
2012-11-02 17:17:07 PDT
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> All reviewed patches have been landed. Closing bug. |