| Summary: | Many CSSOM leaks on fast/dom/non-numeric-values-numeric-parameters.html | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> |
| Component: | CSS | Assignee: | Tadeu Zagallo <tzagallo> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | cdumez, commit-queue, ews-watchlist, ggaren, joepeck, koivisto, rniwa, simon.fraser, tzagallo, webkit-bug-importer, zalan |
| Priority: | P2 | Keywords: | InRadar |
| Version: | Safari 9 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Alexey Proskuryakov
2017-12-01 13:58:54 PST
02/12/2017, 0.04 Simon Fraser: Readable stack: WebCore::jsNodePrototypeFunctionAppendChild(JSC::ExecState*) JSNode.cpp:851 |long long WebCore::IDLOperation<WebCore::JSNode>::call<&(WebCore::jsNodePrototypeFunctionAppendChildBody(JSC::ExecState*, WebCore::JSNode*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) JSDOMOperation.h:53 |WebCore::jsNodePrototypeFunctionAppendChildBody(JSC::ExecState*, WebCore::JSNode*, JSC::ThrowScope&) JSNode.cpp:845 |WebCore::Node::appendChild(WebCore::Node&) Node.cpp:434 |WebCore::ContainerNode::appendChild(WebCore::Node&) ContainerNode.cpp:672 |WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&) ContainerNode.cpp:696 |void WebCore::executeNodeInsertionWithScriptAssertion<WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)::$_4>(WebCore::ContainerNode&, WebCore::Node&, WebCore::ContainerNode::ChildChangeSource, WebCore::ReplacedAllChildren, WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)::$_4) ContainerNode.cpp:183 |WebCore::notifyChildNodeInserted(WebCore::ContainerNode&, WebCore::Node&) ContainerNodeAlgorithms.cpp:99 |WebCore::notifyNodeInsertedIntoDocument(WebCore::ContainerNode&, WebCore::Node&, WebCore::TreeScopeChange, WTF::Vector<WTF::Ref<WebCore::Node>, 11ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) ContainerNodeAlgorithms.cpp:44 |WebCore::HTMLStyleElement::insertedIntoAncestor(WebCore::Node::InsertionType, WebCore::ContainerNode&) HTMLStyleElement.cpp:104 |WebCore::InlineStyleSheetOwner::insertedIntoDocument(WebCore::Element&) InlineStyleSheetOwner.cpp:93 |WebCore::InlineStyleSheetOwner::createSheetFromTextContents(WebCore::Element&) InlineStyleSheetOwner.cpp:134 |WebCore::InlineStyleSheetOwner::createSheet(WebCore::Element&, WTF::String const&) InlineStyleSheetOwner.cpp:207 |WebCore::StyleSheetContents::parseString(WTF::String const&) StyleSheetContents.cpp:357 |WebCore::CSSParser::parseSheet(WebCore::StyleSheetContents*, WTF::String const&, WebCore::CSSParser::RuleParsing) CSSParser.cpp:126 |WebCore::CSSParserImpl::parseStyleSheet(WTF::String const&, WebCore::CSSParserContext const&, WebCore::StyleSheetContents*, WebCore::CSSParser::RuleParsing) CSSParserImpl.cpp:245 |bool WebCore::CSSParserImpl::consumeRuleList<WebCore::CSSParserImpl::parseStyleSheet(WTF::String const&, WebCore::CSSParserContext const&, WebCore::StyleSheetContents*, WebCore::CSSParser::RuleParsing)::$_2>(WebCore::CSSParserTokenRange, WebCore::CSSParserImpl::RuleListType, WebCore::CSSParserImpl::parseStyleSheet(WTF::String const&, WebCore::CSSParserContext const&, WebCore::StyleSheetContents*, WebCore::CSSParser::RuleParsing)::$_2) CSSParserImpl.cpp:387 |WebCore::CSSParserImpl::consumeQualifiedRule(WebCore::CSSParserTokenRange&, WebCore::CSSParserImpl::AllowedRulesType) CSSParserImpl.cpp:473 |WebCore::CSSParserImpl::consumeStyleRule(WebCore::CSSParserTokenRange, WebCore::CSSParserTokenRange) CSSParserImpl.cpp:747 |WebCore::CSSParserImpl::consumeDeclarationList(WebCore::CSSParserTokenRange, WebCore::StyleRuleBase::Type) CSSParserImpl.cpp:780 |WebCore::CSSParserImpl::consumeDeclaration(WebCore::CSSParserTokenRange, WebCore::StyleRuleBase::Type) CSSParserImpl.cpp:842 |WebCore::CSSParserImpl::consumeDeclarationValue(WebCore::CSSParserTokenRange, WebCore::CSSPropertyID, bool, WebCore::StyleRuleBase::Type) CSSParserImpl.cpp:858 |WebCore::CSSPropertyParser::parseValue(WebCore::CSSPropertyID, bool, WebCore::CSSParserTokenRange const&, WebCore::CSSParserContext const&, WTF::Vector<WebCore::CSSProperty, 256ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::StyleRuleBase::Type) CSSPropertyParser.cpp:267 |WebCore::CSSPropertyParser::parseValueStart(WebCore::CSSPropertyID, bool) CSSPropertyParser.cpp:0 |WebCore::CSSPropertyParser::parseSingleValue(WebCore::CSSPropertyID, WebCore::CSSPropertyID) CSSPropertyParser.cpp:3831 |WebCore::consumeFontFamily(WebCore::CSSParserTokenRange&) CSSPropertyParser.cpp:1041 |WebCore::CSSValueList::createCommaSeparated() CSSValueList.h:40 |WTF::RefCounted<WebCore::CSSValue>::operator new(unsigned long) RefCounted.h:140 |WTF::fastMalloc(unsigned long) FastMalloc.cpp:258 Created attachment 337739 [details]
Patch
Comment on attachment 337739 [details] Patch Attachment 337739 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7288071 New failing tests: fast/dom/StyleSheet/gc-inline-style-cssvalues.html fast/dom/gc-9.html Created attachment 337741 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 337739 [details] Patch Attachment 337739 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7288086 New failing tests: fast/dom/StyleSheet/gc-inline-style-cssvalues.html fast/dom/gc-9.html Created attachment 337746 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 337739 [details] Patch Attachment 337739 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7288087 New failing tests: fast/dom/StyleSheet/gc-inline-style-cssvalues.html fast/dom/gc-9.html Created attachment 337750 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 337739 [details] Patch Attachment 337739 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7288085 New failing tests: fast/dom/StyleSheet/gc-inline-style-cssvalues.html fast/dom/gc-9.html Created attachment 337752 [details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 337739 [details] Patch Attachment 337739 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7288983 New failing tests: fast/dom/StyleSheet/gc-inline-style-cssvalues.html fast/dom/gc-9.html Created attachment 337762 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Those tests probably just need rebaselining. Comment on attachment 337739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337739&action=review > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:372 > + UNUSED_PARAM(type); You can just remove the 'type' variable instead. Comment on attachment 337739 [details]
Patch
These failures look like legitimate regressions
-PASS document.body.style.getPropertyCSSValue("color").foo is 'bar'
+FAIL document.body.style.getPropertyCSSValue("color").foo should be bar (of type string). Was undefined (of type undefined).
(In reply to Alexey Proskuryakov from comment #17) > Comment on attachment 337739 [details] > Patch > > These failures look like legitimate regressions > > -PASS document.body.style.getPropertyCSSValue("color").foo is 'bar' > +FAIL document.body.style.getPropertyCSSValue("color").foo should be bar (of > type string). Was undefined (of type undefined). The failure is legitimate, but I believe the test may be misleading: the CSS value is not being collected because of the property being added to it, but if we just use a WeakSet instead, the object is collected and identity is not preserved. But indeed, with this patch, it will always return a new object. Alternatively, we can use weak map to fix the leak and preserve the current behaviour and then discuss whether identity should be preserved or not. The following variation of the 'fast/dom/gc-inline-style-cssvalues.html' test fails on master: var expr = 'document.body.style.getPropertyCSSValue("color")'; var set = new WeakSet(); set.add(eval(expr)); gc(); shouldBe(`set.has(${expr})`, "true"); > These failures look like legitimate regressions
They document what this patch (deliberately) does. Considering Blink removed CSSValue support completely years ago, it is very unlikely that their identity is important.
Assuming we are fine with changing the behaviour, should I remove the failing cases from the tests, update the tests to verify the new behaviour or just update the baseline? I don't think we can break GC behavior here. We should fix GC lifecycle issue. Tadeu and I have already talked about this in person. Created attachment 337936 [details]
Patch
Created attachment 337946 [details]
Patch
Comment on attachment 337946 [details]
Patch
r=me
Comment on attachment 337946 [details] Patch Clearing flags on attachment: 337946 Committed r230737: <https://trac.webkit.org/changeset/230737> All reviewed patches have been landed. Closing bug. |