Bug 128532 – [MSE] Move PublicURLManager shutdown logic so ActiveDOMObjects associated with public URLs won't leak.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Byungseon(Sun) Shin]

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

[Byungseon(Sun) Shin]

Created attachment 223716 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 223779 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 223790 [details]
Patch

[Philippe Normand]

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.

[Byungseon(Sun) Shin]

Created attachment 223850 [details]
Patch

[Jer Noble]

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.

[Byungseon(Sun) Shin]

Created attachment 223867 [details]
Patch

[Byungseon(Sun) Shin]

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.

[Byungseon(Sun) Shin]

Created attachment 223871 [details]
Patch

[Jer Noble]

Comment on attachment 223871 [details]
Patch

Thanks! r=me.

[WebKit Commit Bot]

Comment on attachment 223871 [details]
Patch

Clearing flags on attachment: 223871

Committed r164091: <http://trac.webkit.org/changeset/164091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Byungseon(Sun) Shin]

(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<>.

[Byungseon(Sun) Shin]

(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<>.