Bug 35014

Summary:

Modifying UA rules from page JS crashes

Product:

WebKit

Reporter:

Boris Zbarsky <bzbarsky>

Component:

CSS

Assignee:

Yuzo Fujishima <yuzo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bdakin, mitz, yuzo

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Fix Bug 35014 - Modifying UA rules from page JS crashes	darin: review+

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+	Details Formatted Diff Diff
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.