RESOLVED FIXED 35014
Modifying UA rules from page JS crashes 
https://bugs.webkit.org/show_bug.cgi?id=35014
Summary Modifying UA rules from page JS crashes
Boris Zbarsky
Reported 2010-02-16 19:09:43 PST
STEPS TO REPRODUCE: 1) Start Safari. 2) Load about:blank (or really, any page of your choice) 3) Type : javascript:window.getMatchedCSSRules(document.body, "", false)[0].style.marginTop="200px";void(0); into the url bar. 4) Hit enter.
Attachments
Fix Bug 35014 - Modifying UA rules from page JS crashes (3.25 KB, patch)
2010-05-12 02:08 PDT, Yuzo Fujishima 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mark Rowe (bdash)
Comment 1 2010-02-16 19:15:34 PST
<rdar://problem/7656398>
Alexey Proskuryakov
Comment 2 2010-02-17 14:40:53 PST
Looks like null dereference. Thread 0 Crashed: 0 com.apple.WebCore 0x019a07fb WebCore::Document::updateStyleSelector() + 9 (Document.cpp:2459) 1 com.apple.WebCore 0x018d887a WebCore::CSSMutableStyleDeclaration::setNeedsStyleRecalc() + 232 (CSSMutableStyleDeclaration.cpp:488) 2 com.apple.WebCore 0x018d96d4 WebCore::CSSMutableStyleDeclaration::setProperty(int, WebCore::String const&, bool, bool) + 312 (CSSMutableStyleDeclaration.cpp:541) 3 com.apple.WebCore 0x018d97fe WebCore::CSSMutableStyleDeclaration::setProperty(int, WebCore::String const&, bool, int&) + 62 (CSSMutableStyleDeclaration.cpp:512) 4 com.apple.WebCore 0x01910b4c WebCore::CSSStyleDeclaration::setProperty(WebCore::String const&, WebCore::String const&, WebCore::String const&, int&) + 126 (CSSStyleDeclaration.cpp:101) 5 com.apple.WebCore 0x01910c4b WebCore::CSSStyleDeclaration::setProperty(WebCore::String const&, WebCore::String const&, int&) + 119 (CSSStyleDeclaration.cpp:87) 6 com.apple.WebCore 0x01c85ddc WebCore::JSCSSStyleDeclaration::putDelegate(JSC::ExecState*, JSC::Identifier const&, JSC::JSValue, JSC::PutPropertySlot&) + 242 (JSCSSStyleDeclarationCustom.cpp:186) 7 com.apple.WebCore 0x01c84059 WebCore::JSCSSStyleDeclaration::put(JSC::ExecState*, JSC::Identifier const&, JSC::JSValue, JSC::PutPropertySlot&) + 59 (JSCSSStyleDeclaration.cpp:255) 8 com.apple.JavaScriptCore 0x00e1c914 JSC::JSValue::put(JSC::ExecState*, JSC::Identifier const&, JSC::JSValue, JSC::PutPropertySlot&) + 162 (JSObject.h:646) 9 com.apple.JavaScriptCore 0x00e4c138 cti_op_put_by_id + 156 (JITStubs.cpp:1231) 10 com.apple.JavaScriptCore 0x00e40ff4 jscGeneratedNativeCode + 0 (JITStubs.cpp:932)
Yuzo Fujishima
Comment 3 2010-05-12 02:08:39 PDT
Created attachment 55818 [details] Fix Bug 35014 - Modifying UA rules from page JS crashes
Darin Adler
Comment 4 2010-05-12 11:54:54 PDT
Comment on attachment 55818 [details] Fix Bug 35014 - Modifying UA rules from page JS crashes > +<!doctype html> > +<html> > +<head> > +<title>Test for Bug 35014 - Modifying UA rules from page JS crashes</title> > +</head> > +<body> > +<script> > +if (window.layoutTestController) > + layoutTestController.dumpAsText(); > + > +window.getMatchedCSSRules(document.body, "", false)[0].style.marginTop="200px"; > +</script> > +PASS > +</body> > +</html> I don't like that this test claims to PASS even if it hasn't even run. Typically the test would replace a "test has not run" message with a "test passed if we do not see a crash" message *after* running the line of code in question. > - if (root->isCSSStyleSheet()) > - static_cast<CSSStyleSheet*>(root)->doc()->updateStyleSelector(); > + if (root->isCSSStyleSheet()) { > + Document* doc = static_cast<CSSStyleSheet*>(root)->doc(); > + if (doc) > + doc->updateStyleSelector(); > + } It's great to add a null check. More idiomatic for WebKit would be: if (root->isCSSStyleSheet()) { if (Document* document = static_cast<CSSStyleSheet*>(root)->doc()); document->updateStyleSelector(); } r=me, but this could be improved a bit
Yuzo Fujishima
Comment 5 2010-05-13 00:33:01 PDT
Committed r59351: <http://trac.webkit.org/changeset/59351>
Yuzo Fujishima
Comment 6 2010-05-13 00:36:06 PDT
Thank you for reviewing this. (In reply to comment #4) > (From update of attachment 55818 [details]) > > +<!doctype html> > > +<html> > > +<head> > > +<title>Test for Bug 35014 - Modifying UA rules from page JS crashes</title> > > +</head> > > +<body> > > +<script> > > +if (window.layoutTestController) > > + layoutTestController.dumpAsText(); > > + > > +window.getMatchedCSSRules(document.body, "", false)[0].style.marginTop="200px"; > > +</script> > > +PASS > > +</body> > > +</html> > > I don't like that this test claims to PASS even if it hasn't even run. Typically the test would replace a "test has not run" message with a "test passed if we do not see a crash" message *after* running the line of code in question. Changed as suggested. Also, I changed the test such that the marginTop is reset to the original value after testing. > > > - if (root->isCSSStyleSheet()) > > - static_cast<CSSStyleSheet*>(root)->doc()->updateStyleSelector(); > > + if (root->isCSSStyleSheet()) { > > + Document* doc = static_cast<CSSStyleSheet*>(root)->doc(); > > + if (doc) > > + doc->updateStyleSelector(); > > + } > > It's great to add a null check. More idiomatic for WebKit would be: > > if (root->isCSSStyleSheet()) { > if (Document* document = static_cast<CSSStyleSheet*>(root)->doc()); > document->updateStyleSelector(); > } Changed as suggested. > > r=me, but this could be improved a bit
Alexey Proskuryakov
Comment 7 2010-11-30 16:26:46 PST
This change should have no effect now - getMatchedCSSRules no longer provides access to UA rules.
Note You need to log in before you can comment on or make changes to this bug.