Bug 50990 – Unused CSSRuleList vector in CSSParser class.

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Shane Stephens]

Unused CSSRuleList vector in CSSParser class.

[Shane Stephens]

Created attachment 76460 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 76460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76460&action=review

> WebCore/css/CSSRuleList.h:37
> +    CSSRuleList();

It would be better to use releaseRef() instead of exposing the constructor.

[Shane Stephens]

Created attachment 76963 [details]
Patch

[Shane Stephens]

I used leakRef() instead of releaseRef() as it seems releaseRef() is being removed at some point?

[David Levin]

Comment on attachment 76963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76963&action=review

Overall, this feel tricky. (Leaking the pointer and then in another function in another file doing the adoptRef.)  Reinforcing this is the fact that is there is a memory leak bug which several of us missed.  (Eric missed it. I didn't see it for a while, and you missed it.)

So I'm wondering what benefit is it to remove this variable? (It looks like CSSParser is not a long lived object. It also doesn't appear that lots of them are created at the same time.)

> WebCore/css/CSSGrammar.y:415
> +          if (!$$) {

Nit: No { for single line statements.

> WebCore/css/CSSParser.cpp:5518
>      if (!media || !rules || !m_styleSheet)

The ref count on rules leaks here.

I'd suggest putting rules in a RefPtr<> = adoptRef(rules); immediately upon entering this function and then doing a .releaseRef() below.

[Ahmad Saleem]

CSSGrammer.y does not exist on Webkit Github repo as of today.

Further, in CSSParser.cpp, there is no mention of createMediaRule and nothing about createRuleList:


https://github.com/WebKit/WebKit/blob/a8766e9cb3f1d5446226413afe0de6b0a64dc996/Source/WebCore/css/parser/CSSParser.cpp

I think CSSPraser has been changed significantly since this patch and it cannot be applied directly. I am going to mark this as "RESOLVED WONTFIX". Please add anything for future reference or if this is incorrect conclusion. Thanks!