Bug 145593 - Make CSSStyleDeclaration to reference countable
Summary: Make CSSStyleDeclaration to reference countable
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-02 22:23 PDT by Hyungwook Lee
Modified: 2015-06-10 17:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.66 KB, patch)
2015-06-02 22:24 PDT, Gyuyoung Kim
koivisto: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyungwook Lee 2015-06-02 22:23:45 PDT
CSSStyleDeclaration class and its child classes have handled reference count manually. I think it would be good if we delegate the reference count to RefCount framework.
Comment 1 Gyuyoung Kim 2015-06-02 22:24:39 PDT
Created attachment 254148 [details]
Patch
Comment 2 Darin Adler 2015-06-03 13:37:07 PDT
Comment on attachment 254148 [details]
Patch

Andreas or Antti did this intentionally, perhaps to save memory. One of them needs to review this.
Comment 3 Antti Koivisto 2015-06-05 06:41:47 PDT
Comment on attachment 254148 [details]
Patch

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

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:-129
> -void PropertySetCSSStyleDeclaration::ref()
> -{ 
> -    m_propertySet->ref();
> -}
> -
> -void PropertySetCSSStyleDeclaration::deref()
> -{
> -    m_propertySet->deref(); 
> -}

These types have custom refcounting so we can do this here. Basically the refcount of PropertySetCSSStyleDeclaration is delegated to the underlying property set. This scheme is used so CSSStyleDeclaration wrappers for property sets keep their script-observable identity.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:76
> -    MutableStyleProperties* m_propertySet;
> +    RefPtr<MutableStyleProperties> m_propertySet;

This creates an ownership cycle. MutableStyleProperties has

std::unique_ptr<PropertySetCSSStyleDeclaration> m_cssomWrapper;

This will prevent it from dying and we'll leak MutableStyleProperties/PropertySetCSSStyleDeclaration pairs.
Comment 4 Gyuyoung Kim 2015-06-10 17:48:36 PDT
(In reply to comment #3)
> Comment on attachment 254148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254148&action=review
> 
> > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:-129
> > -void PropertySetCSSStyleDeclaration::ref()
> > -{ 
> > -    m_propertySet->ref();
> > -}
> > -
> > -void PropertySetCSSStyleDeclaration::deref()
> > -{
> > -    m_propertySet->deref(); 
> > -}
> 
> These types have custom refcounting so we can do this here. Basically the
> refcount of PropertySetCSSStyleDeclaration is delegated to the underlying
> property set. This scheme is used so CSSStyleDeclaration wrappers for
> property sets keep their script-observable identity.
> 
> > Source/WebCore/css/PropertySetCSSStyleDeclaration.h:76
> > -    MutableStyleProperties* m_propertySet;
> > +    RefPtr<MutableStyleProperties> m_propertySet;
> 
> This creates an ownership cycle. MutableStyleProperties has
> 
> std::unique_ptr<PropertySetCSSStyleDeclaration> m_cssomWrapper;
> 
> This will prevent it from dying and we'll leak
> MutableStyleProperties/PropertySetCSSStyleDeclaration pairs.

Thank you for your review !