Bug 153462

Created attachment 269835 [details] Patch

When did this start?

(In reply to comment #2) > When did this start? Several months ago. It's not the only build warning we have, and the last time I looked I wasn't sure what to do with it. I think the warning is only available in newer versions of bison than you'd have easy access to.

It was introduced in r191128, so I'll CC David for review. P.S. My rationale for CCing you, Alex, was that I once heard you say "bison."

http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar.y.in definitely added some new's without a corresponding delete. I'd prefer to have someone double check this, though. Hyatt?

*** Bug 150325 has been marked as a duplicate of this bug. ***

This code is definitely run by fast/css/variables/test-suite/036.html and fast/css/variables/test-suite/068.html

Comment on attachment 269835 [details] Patch Running those tests with guardmalloc passes, so I'm going to say r=me

(In reply to comment #5) > http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar. > y.in definitely added some new's without a corresponding delete. And this delete does not correspond to the new's added in that patch.

Comment on attachment 269835 [details] Patch Clearing flags on attachment: 269835 Committed r195612: <http://trac.webkit.org/changeset/195612>

All reviewed patches have been landed. Closing bug.

(In reply to comment #9) > (In reply to comment #5) > > http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar. > > y.in definitely added some new's without a corresponding delete. > And this delete does not correspond to the new's added in that patch. This delete corresponds with the following new in line 1668 in the definition of valid_expr: $$ = new CSSParserValueList; Correct me if I'm wrong.

Comment on attachment 269835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269835&action=review > Source/WebCore/ChangeLog:12 > + "Right-hand side symbols of a rule that explicitly triggers a syntax error via YYERROR are > + not discarded automatically. As a rule of thumb, destructors are invoked only when user > + actions cannot manage the memory." Sounds like we might have leaks in other rules that invoke YYERROR; did you check those over?

(In reply to comment #13) > Sounds like we might have leaks in other rules that invoke YYERROR; did you > check those over? I looked them over, and I'm pretty sure there isn't one we're missing a delete before. Most of them would have a similar warning, and the ones that wouldn't (because $1, $2, etc. are used somewhere else) have entries like IDENT (which I think is a string) or a nested_selector_list that is verified to be nullptr (and it's a unique_ptr anyways). Maybe we should use unique_ptrs everywhere in the parser instead of new to avoid future problems like this.

(In reply to comment #13) > Sounds like we might have leaks in other rules that invoke YYERROR; did you > check those over? I think the other YYERROR cases are fine; there are not that many. (In reply to comment #12) > This delete corresponds with the following new in line 1668 in the > definition of valid_expr: > $$ = new CSSParserValueList; > > Correct me if I'm wrong. I am not sure whether it's guaranteed to come from there; grammars are hard and I don't really want to try to understand this one. What's important is that expr has a semantic value of type CSSParserValueList*, so each rule that uses expr needs to either delete it or percolate it up by assigning it to $$, else it will be leaked. (In reply to comment #14) > I looked them over, and I'm pretty sure there isn't one we're missing a > delete before. Most of them would have a similar warning, and the ones that > wouldn't (because $1, $2, etc. are used somewhere else) have entries like > IDENT (which I think is a string) or a nested_selector_list that is verified > to be nullptr (and it's a unique_ptr anyways). Maybe we should use > unique_ptrs everywhere in the parser instead of new to avoid future problems > like this. This would make the bison parser more robust, to be sure. (I am worried that the current grammar might be leaking values in cases where they are actually used, which would suppress the warning. But maybe it is fine; I don't plan to investigate further.) There might be an easy way to use std::unique_ptr. I have heard that it might be possible in C++ 11 to use class instances in unions (and have them be properly destructed), but I am having trouble finding reliable info with a quick Internet search. If that's true, and it works in GCC/Clang/MSVCC, then we could use std::unique_ptr in the %union declaration at the top of the file, which would allow us to get some safety without switching to a different skeleton. I am not sure if it is possible to safely use class types in unions, though; if not, we are stuck with error-prone new and delete. There is an alternative, which is to switch to the experimental C++ skeleton. The new skeleton allows storing C++ objects as semantic values, rather than relying on a union. Unfortunately, it is only available in modern versions of bison. I believe you would have internal political difficulties to do this, due to the GPLv3+ license, although the same non-infectious license exception applies to the generated code as always (so we can safely use it in WebKit without infecting our license). I hesitate to recommend this, though, as the C++ skeleton is still experimental; some serious bugs in it that I discovered were fixed not so long ago, and my grammar was for a school project, much simpler than WebKit's.

I think the right solution is to not use bison.

I'm a fan of bison. *Shrug* I looked up unions in "The C++ Programming Language" and the answer is: yes you can use class types in unions now, but it's not going to work for std::unique_ptr because it requires no custom contructor, destructor, or copy/move assignment operators.