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.
<rdar://problem/7656398>
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)
Created attachment 55818 [details] Fix Bug 35014 - Modifying UA rules from page JS crashes
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
Committed r59351: <http://trac.webkit.org/changeset/59351>
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
This change should have no effect now - getMatchedCSSRules no longer provides access to UA rules.