Bug 120479 – Possible dangling CachedResourceClient of StyleRuleImport and XSLImportRule

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Leo Yang]

In StyleRuleImport::requestStyleSheet() and XSLImportRule::loadSheet() we don't call removeClient() for m_cachedSheet before assign m_cachedSheet a new value. This could leave *this* as a client of the old cached sheet and dangling after *this* is deleted.

[Leo Yang]

Created attachment 210003 [details]
Patch

[Darin Adler]

Comment on attachment 210003 [details]
Patch

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

Fix looks good, but needs a test or an explanation of why there is no test.

> Source/WebCore/ChangeLog:14
> +        No functionalities changed no new tests.

This fixes a bug. It’s not right to say “no functionalities changed”. I am guessing you noticed the bug by code inspection. Once you knew the bug existed, did you try to make a test case? Why were you unable to do so? The comment here should address those questions, rather than incorrectly saying that nothing was changed.

[Leo Yang]

Comment on attachment 210003 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        No functionalities changed no new tests.
> 
> This fixes a bug. It’s not right to say “no functionalities changed”. I am guessing you noticed the bug by code inspection. Once you knew the bug existed, did you try to make a test case? Why were you unable to do so? The comment here should address those questions, rather than incorrectly saying that nothing was changed.

Yes I found the bug by code inspection. I tried to make a test case but I don't know how I can manipulate import rules by Javascript. Is there a way to do that or should I just say no way to test it automatically?

[Leo Yang]

Created attachment 210102 [details]
Patch v2

[Darin Adler]

Comment on attachment 210102 [details]
Patch v2

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

> Source/WebCore/ChangeLog:14
> +        Found by code inspection. Seems no way to test it automatically.

I don’t think we should give up on figuring out what the actual symptom of this is. I suspect eventually we could come up with a way to test it.

[WebKit Commit Bot]

Comment on attachment 210102 [details]
Patch v2

Clearing flags on attachment: 210102

Committed r154889: <http://trac.webkit.org/changeset/154889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.