CSSParserVariable leaks seen on leaks bots

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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;
> }

[Joseph Pecoraro]

* STEPS TO REPRODUCE
shell> run-webkit-tests --leaks -1 fast/css/variables

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 264410 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 264410 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 264410

Committed r191825: <http://trac.webkit.org/changeset/191825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.