150724 2015-10-30 11:52:31 -0700 CSSParserVariable leaks seen on leaks bots 2015-10-30 16:19:21 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 joepeck joepeck ap commit-queue darin hyatt joepeck oldest_to_newest 1137906 0 joepeck 2015-10-30 11:52:31 -0700 * SUMMARY CSSParserVariable leaks seen on leaks bots. https://build.webkit.org/builders/Apple%20Yosemite%20%28Leaks%29/builds/3581 * LEAK (allocation trace) Call stack: [thread 0x11c62f300]: | 0x2 | start | main DumpRenderTreeMain.mm:30 | DumpRenderTreeMain(int, char const**) DumpRenderTree.mm:1430 | dumpRenderTree(int, char const**) DumpRenderTree.mm:1295 | runTestingServerLoop() DumpRenderTree.mm:1186 | runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:2036 | CFRunLoopRunSpecific | __CFRunLoopRun | __CFRunLoopDoSources0 | __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ | MultiplexerSource::_perform(void*) | MultiplexerSource::perform() | RunloopBlockContext::perform() | CFArrayApplyFunction | RunloopBlockContext::_invoke_block(void const*, void*) | ___ZN27URLConnectionClient_Classic18_withDelegateAsyncEPKcU13block_pointerFvP16_CFURLConnectionPK33CFURLConnectionClientCurrent_VMaxE_block_invoke_2 | ___ZN27URLConnectionClient_Classic26_delegate_didFinishLoadingEU13block_pointerFvvE_block_invoke | -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] | -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] | __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] WebCoreResourceHandleAsDelegate.mm:258 | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) ResourceLoader.cpp:639 | WebCore::SubresourceLoader::didFinishLoading(double) SubresourceLoader.cpp:374 | WebCore::CachedCSSStyleSheet::finishLoading(WebCore::SharedBuffer*) CachedCSSStyleSheet.cpp:101 | WebCore::CachedCSSStyleSheet::checkNotify() CachedCSSStyleSheet.cpp:113 | non-virtual thunk to WebCore::HTMLLinkElement::setCSSStyleSheet(WTF::String const&, WebCore::URL const&, WTF::String const&, WebCore::CachedCSSStyleSheet const*) HTMLLinkElement.cpp:359 | WebCore::HTMLLinkElement::setCSSStyleSheet(WTF::String const&, WebCore::URL const&, WTF::String const&, WebCore::CachedCSSStyleSheet const*) HTMLLinkElement.cpp:351 | WebCore::StyleSheetContents::parseAuthorStyleSheet(WebCore::CachedCSSStyleSheet const*, WebCore::SecurityOrigin const*) StyleSheetContents.cpp:317 | WebCore::CSSParser::parseSheet(WebCore::StyleSheetContents*, WTF::String const&, WTF::TextPosition const&, WTF::Vector<WTF::RefPtr<WebCore::CSSRuleSourceData>, 0ul, WTF::CrashOnOverflow, 16ul>*, bool) CSSParser.cpp:462 | cssyyparse(WebCore::CSSParser*) .CSSGrammar.y:1743 | WebCore::CSSParserVariable::operator new(unsigned long) CSSParserValues.h:193 | WTF::fastMalloc(unsigned long) FastMalloc.cpp:193 | bmalloc::api::malloc(unsigned long) bmalloc.h:43 | bmalloc::Cache::allocate(unsigned long) Cache.h:79 | bmalloc::Allocator::allocate(unsigned long) Allocator.h:86 | bmalloc::Allocator::allocateSlowCase(unsigned long) Allocator.cpp:242 | malloc | malloc_zone_malloc * NOTES CSSGrammar.y.in > variable_function: > VARFUNCTION maybe_space CUSTOM_PROPERTY maybe_space closing_parenthesis { > CSSParserVariable* var = new CSSParserVariable; > var->name = $3; > var->args = nullptr; > $$.id = CSSValueInvalid; > $$.unit = CSSParserValue::Variable; > $$.variable = var; > } > ... It looks like `variable_function` has some destroy() method: > %type <value> calc_function function variable_function min_or_max_function term > %destructor { destroy($$); } calc_function function variable_function min_or_max_function term Maybe this destroy function should be deleting `.variable` in the new case: > void destroy(const CSSParserValue& value) > { > if (value.unit == CSSParserValue::Function) > delete value.function; > else if (value.unit == CSSParserValue::ValueList) > delete value.valueList; > } 1137914 1 joepeck 2015-10-30 12:02:31 -0700 * STEPS TO REPRODUCE shell> run-webkit-tests --leaks -1 fast/css/variables 1137920 2 joepeck 2015-10-30 12:18:55 -0700 This gets most of the leaks, there are still others. I'll give some time to tracking them otherwise I'll just put this patch up. 1137923 3 joepeck 2015-10-30 12:28:19 -0700 There is a CSSParserValueList leak, which I suspect is happening from: CSSValueList::buildParserValueSubstitutingVariables CSSValueList::buildParserValueListSubstitutingVariables They call each other but in the error cases I don't think the CSSParserValueList created by buildParserValueSubstitutingVariables gets destroyed. I'll file a separate bug about this, since it is non-trivial for me but probably easy for those familiar with what this code is doing. 1137930 4 264410 joepeck 2015-10-30 12:37:02 -0700 Created attachment 264410 [PATCH] Proposed Fix 1138018 5 264410 commit-queue 2015-10-30 16:19:17 -0700 Comment on attachment 264410 [PATCH] Proposed Fix Clearing flags on attachment: 264410 Committed r191825: <http://trac.webkit.org/changeset/191825> 1138019 6 commit-queue 2015-10-30 16:19:21 -0700 All reviewed patches have been landed. Closing bug. 264410 2015-10-30 12:37:02 -0700 2015-10-30 16:19:17 -0700 [PATCH] Proposed Fix variables-cleanup.patch text/plain 1205 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzNGVlNjMwLi43YWU5ZjlmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTYg QEAKIDIwMTUtMTAtMzAgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAor ICAgICAgICBDU1NQYXJzZXJWYXJpYWJsZSBsZWFrcyBzZWVuIG9uIGxlYWtzIGJvdHMKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MDcyNAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogY3NzL0NTU1BhcnNl clZhbHVlcy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpkZXN0cm95KToKKyAgICAgICAgQ2xlYW51 cCB2YXJpYWJsZSBDU1NQYXJzZXJWYWx1ZXMuCisKKzIwMTUtMTAtMzAgIEpvc2VwaCBQZWNvcmFy byAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwogICAgICAgICBNaW5vciBDR0NvbG9yIGxlYWtzIHNl ZW4gb24gYm90cyBhbGxvY2F0ZWQgaW4gV2ViU3lzdGVtQmFja2Ryb3BMYXllci5tbQogICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTUwNzIyCiAKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXJWYWx1ZXMuY3BwIGIvU291cmNlL1dl YkNvcmUvY3NzL0NTU1BhcnNlclZhbHVlcy5jcHAKaW5kZXggNmIwZDZmZi4uN2NjMzkzZiAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvY3NzL0NTU1BhcnNlclZhbHVlcy5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvY3NzL0NTU1BhcnNlclZhbHVlcy5jcHAKQEAgLTQwLDYgKzQwLDggQEAgdm9p ZCBkZXN0cm95KGNvbnN0IENTU1BhcnNlclZhbHVlJiB2YWx1ZSkKICAgICAgICAgZGVsZXRlIHZh bHVlLmZ1bmN0aW9uOwogICAgIGVsc2UgaWYgKHZhbHVlLnVuaXQgPT0gQ1NTUGFyc2VyVmFsdWU6 OlZhbHVlTGlzdCkKICAgICAgICAgZGVsZXRlIHZhbHVlLnZhbHVlTGlzdDsKKyAgICBlbHNlIGlm ICh2YWx1ZS51bml0ID09IENTU1BhcnNlclZhbHVlOjpWYXJpYWJsZSkKKyAgICAgICAgZGVsZXRl IHZhbHVlLnZhcmlhYmxlOwogfQogCiBDU1NQYXJzZXJWYWx1ZUxpc3Q6On5DU1NQYXJzZXJWYWx1 ZUxpc3QoKQo=