35014 2010-02-16 19:09:43 -0800 Modifying UA rules from page JS crashes 2010-11-30 16:26:46 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED InRadar P1 Normal --- 1 bzbarsky yuzo ap bdakin mitz yuzo oldest_to_newest 190859 0 bzbarsky 2010-02-16 19:09:43 -0800 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. 190861 1 mrowe 2010-02-16 19:15:34 -0800 <rdar://problem/7656398> 191227 2 ap 2010-02-17 14:40:53 -0800 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) 224480 3 55818 yuzo 2010-05-12 02:08:39 -0700 Created attachment 55818 Fix Bug 35014 - Modifying UA rules from page JS crashes 224775 4 55818 darin 2010-05-12 11:54:54 -0700 Comment on attachment 55818 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 225186 5 yuzo 2010-05-13 00:33:01 -0700 Committed r59351: <http://trac.webkit.org/changeset/59351> 225188 6 yuzo 2010-05-13 00:36:06 -0700 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 315383 7 ap 2010-11-30 16:26:46 -0800 This change should have no effect now - getMatchedCSSRules no longer provides access to UA rules. 55818 2010-05-12 02:08:39 -0700 2010-05-12 11:54:54 -0700 Fix Bug 35014 - Modifying UA rules from page JS crashes bug-35014-20100512020837.patch text/plain 3326 yuzo ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCA1NDBmMjQwYTMyMDkxNWUzMTU3M2VmMWE0NTU2NGNiZTFlOGJlMWZmLi4wZTVhMzRi ZGNkNjVjNWJhNDYwYTg0ZTM2NmZjZjA3MTYyNTlmMjU2IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0 cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTAtMDUtMTIgIFl1em8gRnVqaXNoaW1hICA8eXV6b0Bnb29nbGUuY29tPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeCBCdWcgMzUwMTQgLSBN b2RpZnlpbmcgVUEgcnVsZXMgZnJvbSBwYWdlIEpTIGNyYXNoZXMKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM1MDE0CisKKyAgICAgICAgKiBmYXN0L2Nz cy9tb2RpZnktdWEtcnVsZXMtZnJvbS1qYXZhc2NyaXB0LWV4cGVjdGVkLnR4dDogQ29waWVkIGZy b20gTGF5b3V0VGVzdHMvZWRpdGluZy9zZWxlY3Rpb24vNTEzNjY5Ni1leHBlY3RlZC50eHQuCisg ICAgICAgICogZmFzdC9jc3MvbW9kaWZ5LXVhLXJ1bGVzLWZyb20tamF2YXNjcmlwdC5odG1sOiBB ZGRlZC4KKwogMjAxMC0wNS0xMCAgRXJpYyBTZWlkZWwgIDxlcmljQHdlYmtpdC5vcmc+CiAKICAg ICAgICAgVW5yZXZpZXdlZCwganVzdCByZW1vdmluZyBuZXdsaW5lIGZyb20gLWV4cGVjdGVkLnR4 dCBmaWxlCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9tb2RpZnktdWEtcnVsZXMt ZnJvbS1qYXZhc2NyaXB0LWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL21vZGlm eS11YS1ydWxlcy1mcm9tLWphdmFzY3JpcHQtZXhwZWN0ZWQudHh0Cm5ldyBmaWxlIG1vZGUgMTAw NjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjdlZjIy ZTlhNDMxYWQwMjcyNzEzYjcxZmRjODc5NDAxNmM4ZWYxMmYKLS0tIC9kZXYvbnVsbAorKysgYi9M YXlvdXRUZXN0cy9mYXN0L2Nzcy9tb2RpZnktdWEtcnVsZXMtZnJvbS1qYXZhc2NyaXB0LWV4cGVj dGVkLnR4dApAQCAtMCwwICsxIEBACitQQVNTCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0 L2Nzcy9tb2RpZnktdWEtcnVsZXMtZnJvbS1qYXZhc2NyaXB0Lmh0bWwgYi9MYXlvdXRUZXN0cy9m YXN0L2Nzcy9tb2RpZnktdWEtcnVsZXMtZnJvbS1qYXZhc2NyaXB0Lmh0bWwKbmV3IGZpbGUgbW9k ZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMC4u ZDlkNGZmNmU0ZDZiMTMwZDQ2OTNmNDQ4ZTdkYjFiMmJlN2MwMWEzZAotLS0gL2Rldi9udWxsCisr KyBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL21vZGlmeS11YS1ydWxlcy1mcm9tLWphdmFzY3JpcHQu aHRtbApAQCAtMCwwICsxLDE1IEBACis8IWRvY3R5cGUgaHRtbD4KKzxodG1sPgorPGhlYWQ+Cis8 dGl0bGU+VGVzdCBmb3IgQnVnIDM1MDE0IC0gTW9kaWZ5aW5nIFVBIHJ1bGVzIGZyb20gcGFnZSBK UyBjcmFzaGVzPC90aXRsZT4KKzwvaGVhZD4KKzxib2R5PgorPHNjcmlwdD4KK2lmICh3aW5kb3cu bGF5b3V0VGVzdENvbnRyb2xsZXIpCisgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIuZHVtcEFzVGV4 dCgpOworCit3aW5kb3cuZ2V0TWF0Y2hlZENTU1J1bGVzKGRvY3VtZW50LmJvZHksICIiLCBmYWxz ZSlbMF0uc3R5bGUubWFyZ2luVG9wPSIyMDBweCI7Cis8L3NjcmlwdD4KK1BBU1MKKzwvYm9keT4K KzwvaHRtbD4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VM b2cKaW5kZXggMWRiZmY1NThlYjdlNTZlNWJjY2E5N2M5ZjRmZmI5ZjZjNTVkMDYxMC4uNjkzZThk NjI3YmI2MDJiOWRkNmIwMmUwNTY5Y2M2ZDE0ZTdmODQyZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9D aGFuZ2VMb2cKKysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAxMC0w NS0xMiAgWXV6byBGdWppc2hpbWEgIDx5dXpvQGdvb2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4IEJ1ZyAzNTAxNCAtIE1vZGlmeWlu ZyBVQSBydWxlcyBmcm9tIHBhZ2UgSlMgY3Jhc2hlcworICAgICAgICBBZGRlZCBhIE5VTEwgY2hl Y2suCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNTAx NAorCisgICAgICAgIFRlc3Q6IGZhc3QvY3NzL21vZGlmeS11YS1ydWxlcy1mcm9tLWphdmFzY3Jp cHQuaHRtbAorCisgICAgICAgICogY3NzL0NTU011dGFibGVTdHlsZURlY2xhcmF0aW9uLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkNTU011dGFibGVTdHlsZURlY2xhcmF0aW9uOjpzZXROZWVkc1N0 eWxlUmVjYWxjKToKKwogMjAxMC0wNS0xMCAgRHVtaXRydSBEYW5pbGl1YyAgPGR1bWlAY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEJyYWR5IEVpZHNvbi4KZGlmZiAtLWdpdCBh L1dlYkNvcmUvY3NzL0NTU011dGFibGVTdHlsZURlY2xhcmF0aW9uLmNwcCBiL1dlYkNvcmUvY3Nz L0NTU011dGFibGVTdHlsZURlY2xhcmF0aW9uLmNwcAppbmRleCA3YzA5ZjkxYzdlNGY2NTE0NGEw NzQ5NDY3NTJkMDllNWJlMTE3YzRiLi5mNWVmMDRhOTE1YTQ4MjVlOWNiNDBlZGUxMWMxMGYyYTkz ZWE5MDcwIDEwMDY0NAotLS0gYS9XZWJDb3JlL2Nzcy9DU1NNdXRhYmxlU3R5bGVEZWNsYXJhdGlv bi5jcHAKKysrIGIvV2ViQ29yZS9jc3MvQ1NTTXV0YWJsZVN0eWxlRGVjbGFyYXRpb24uY3BwCkBA IC00ODQsOCArNDg0LDExIEBAIHZvaWQgQ1NTTXV0YWJsZVN0eWxlRGVjbGFyYXRpb246OnNldE5l ZWRzU3R5bGVSZWNhbGMoKQogICAgIFN0eWxlQmFzZSogcm9vdCA9IHRoaXM7CiAgICAgd2hpbGUg KFN0eWxlQmFzZSogcGFyZW50ID0gcm9vdC0+cGFyZW50KCkpCiAgICAgICAgIHJvb3QgPSBwYXJl bnQ7Ci0gICAgaWYgKHJvb3QtPmlzQ1NTU3R5bGVTaGVldCgpKQotICAgICAgICBzdGF0aWNfY2Fz dDxDU1NTdHlsZVNoZWV0Kj4ocm9vdCktPmRvYygpLT51cGRhdGVTdHlsZVNlbGVjdG9yKCk7Cisg ICAgaWYgKHJvb3QtPmlzQ1NTU3R5bGVTaGVldCgpKSB7CisgICAgICAgIERvY3VtZW50KiBkb2Mg PSBzdGF0aWNfY2FzdDxDU1NTdHlsZVNoZWV0Kj4ocm9vdCktPmRvYygpOworICAgICAgICBpZiAo ZG9jKQorICAgICAgICAgICAgZG9jLT51cGRhdGVTdHlsZVNlbGVjdG9yKCk7CisgICAgfQogfQog CiBib29sIENTU011dGFibGVTdHlsZURlY2xhcmF0aW9uOjpnZXRQcm9wZXJ0eVByaW9yaXR5KGlu dCBwcm9wZXJ0eUlEKSBjb25zdAo=