Converted PublicURLManager to an ActiveDOMObject so that it will release all of its references when all other ActiveDOMObjects are stopped. This prevents ActiveDOMObjects that can be associate with public URLs, like WebKitMediaSource, from getting leaked. Shutting down earlier allows these objects to release their pending activity so V8 can garbage collect their wrappers. When the wrappers get cleaned up the Document is then able to be destroyed. Merged from Blink: https://src.chromium.org/viewvc/blink?view=rev&revision=151890
Created attachment 223716 [details] Patch
Created attachment 223779 [details] Patch
Created attachment 223790 [details] Patch
Comment on attachment 223790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223790&action=review That looks good but I'd prefer Jer to do the official review, he has better knowledge of MSE > Source/WebCore/ChangeLog:3 > + [MSE] Move PublicURLManager shutdown logic so ActiveDOMObjects associated with public URLs won't leak. Please mention the blink patch from which this patch is based on.
Created attachment 223850 [details] Patch
Comment on attachment 223850 [details] Patch 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.
Created attachment 223867 [details] Patch
Comment on attachment 223850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223850&action=review >> Source/WebCore/ChangeLog:6 >> + 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(). @Jer, thanks for the comment. I have applied new patch. >> Source/WebCore/html/PublicURLManager.h:48 >> + 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. OK, I'll prepare a new patch for it.
Created attachment 223871 [details] Patch
Comment on attachment 223871 [details] Patch Thanks! r=me.
Comment on attachment 223871 [details] Patch Clearing flags on attachment: 223871 Committed r164091: <http://trac.webkit.org/changeset/164091>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > (From update of attachment 223850 [details]) > 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. Bug 128891 has been created to return std::unique_ptr<>.