[webkit-reviews] review granted: [Bug 128532] [MSE] Move PublicURLManager shutdown logic so ActiveDOMObjects associated with public URLs won't leak. : [Attachment 223850] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Byungseon Shin <sun.shin at lge.com>'s
request for review:
Bug 128532: [MSE] Move PublicURLManager shutdown logic so ActiveDOMObjects
associated with public URLs won't leak.
https://bugs.webkit.org/show_bug.cgi?id=128532

Attachment 223850: Patch
https://bugs.webkit.org/attachment.cgi?id=223850&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223850&action=review


> Source/WebCore/ChangeLog:6
> +	   [MSE] Move PublicURLManager shutdown logic so ActiveDOMObjects
associated with public URLs won't leak.
> +	   https://bugs.webkit.org/show_bug.cgi?id=128532
> +
> +	   Reviewed by NOBODY (OOPS!).

I was really confused until I read the bug report.  It would be nice to add
some text to the ChangeLog which says what this fix does.  Something like:

This fixes a leak of DOM objects by breaking the circular reference between
Document, PublicURLManager, and MediaSource. Instead of clearing
PublicURLManager at destruction-time, which is delayed indefinitely because of
the circular reference, clear the PublicURLManager during
ActiveDOMObject::stop().

> Source/WebCore/html/PublicURLManager.h:48
> -    static OwnPtr<PublicURLManager> create() { return adoptPtr(new
PublicURLManager); }
> +    static PassOwnPtr<PublicURLManager> create(ScriptExecutionContext*);

We should fix this function to return a std::unique_ptr<>, but we don't have to
do it in this patch.