StyleSheetList should be a ContextDestructionObserver Requested by abarth on #webkit.
Created attachment 187234 [details] Patch
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.
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.
Created attachment 188457 [details] Patch
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
> I think that it is not ok to call document()->styleSheetCollection()->styleSheetsForStyleSheetList() at this point in the Document's destructor. Can you explain why?
(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.
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.