Bug 99241 – StyleSheetList should be a ContextDestructionObserver

• Status: NEW
• Component: New Bugs

[WebKit Review Bot]

StyleSheetList should be a ContextDestructionObserver
Requested by abarth on #webkit.

[Hanyee Kim]

Created attachment 187234 [details]
Patch

[Adam Barth]

Comment on attachment 187234 [details]
Patch

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

> Source/WebCore/css/StyleSheetList.cpp:61
> +void StyleSheetList::contextDestroyed()
> +{
> +    m_detachedStyleSheets = document()->styleSheetCollection()->styleSheetsForStyleSheetList();
>  }

You need to call the base class here otherwise ContextDestructionObserver's pointer won't be cleared.

Also, is it ok to call document()->styleSheetCollection()->styleSheetsForStyleSheetList() at this point in the Document's destructor?  Your patch might have moved when this happens slightly.

[Hanyee Kim]

Thanks for your comment.

I think that it is not ok to call document()->styleSheetCollection()->styleSheetsForStyleSheetList() at this point in the Document's destructor.
So, I don't remove detachFromDoucment() for store m_detachedStyleSheets.

I will upload a new patch.

[Hanyee Kim]

Created attachment 188457 [details]
Patch

[Build Bot]

Comment on attachment 188457 [details]
Patch

Attachment 188457 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16598048

New failing tests:
media/video-controls-captions-trackmenu.html

[Adam Barth]

> I think that it is not ok to call document()->styleSheetCollection()->styleSheetsForStyleSheetList() at this point in the Document's destructor.

Can you explain why?

[Hanyee Kim]

(In reply to comment #6)
Sorry. I am writing in the wrong.
It is ok to call document()->styleSheetCollection()->styleSheetsForStyleSheetList() in the Document's destructor. But it is not ok to document()->styleSheetCollection()->styleSheetsForStyleSheetList() in StyleSheetList::contextDestroyed().

Because StyleSheetList::contextDestroyed() is called by the ScriptExecutionContext's destructor. after the Document's destructor. Calling document() in contextDestroyed(). So, StyleSheetList::detachFromDocument() is needed  for store m_detachedStyleSheets.

[Darin Adler]

Comment on attachment 188457 [details]
Patch

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

Seems OK, but I’m not sure this refactoring is sufficient improvement to be worth taking the change. If we wanted to take the full benefit from this, I think we’d want to remove the detachFromDocument function too. Also, I think we’d probably want to inline the document() function.

> Source/WebCore/ChangeLog:11
> +        This patch removes the raw pointer of Document in StyleSheetList. It
> +        could be replaced with ContextDestructionObserver.
> +        ContextDestructionObserver has the pointer and clears the pointer
> +        automatically when the document is destroyed.

Why? If this is a cleanup we should do more to actually clean up.

> Source/WebCore/css/StyleSheetList.cpp:53
> +    ScriptExecutionContext* context = ContextDestructionObserver::scriptExecutionContext();

We should call just scriptExecutionContext here, not explicitly ContextDestructionObserver::scriptExecutionContext.

> Source/WebCore/css/StyleSheetList.h:36
> +class StyleSheetList : public RefCounted<StyleSheetList>, public ContextDestructionObserver {

I think we’d want to use private inheritance here, not public.

> Source/WebCore/css/StyleSheetList.h:39
>      ~StyleSheetList();

We’d want to mark this virtual since it now will be implicitly virtual.