1421004 - Channels loading media as a top-level document are all leaked

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Honza Bambas (:mayhemer)]

STR:
1. load http://icecast.unitedradio.it/Virgin.mp3 directly
2. let it play a bit (few secs)
3. close the tab (but leave the browser running)
4. give it some time (1 minute) to confirm that GC when kicks in doesn't release
5. close the browser

The loading channel (http channel) is leaked because it's in Suspend() + Cancel() state and held by its consumers indefinitely.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm making this a P2 on the assumption that it isn't difficult. If it proves to be difficult, feel free to reprioritise.

Comment 2

[Benjamin Chen [:bechen]]

It's easily reproduced, the MediaElement had remove the "self reference" but not go into destructor.
So the problem may point to the MediaDocument/VideoDocument, it seems that the MediaDocumentStreamListener doesn't handle the shutdown case properly.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review commit: https://reviewboard.mozilla.org/r/204490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204490/

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204490/diff/1-2/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

https://reviewboard.mozilla.org/r/204490/#review210050

LGTM. But we need a DOM peer to make sure 'domwindowclosed' is the event we need to break the reference cycle.

Comment 6

[Samael Wang [:freesamael] (away for now)]

If you're relying on `domwindowclosed`, you'll need to at least check `aSubject` is the outerWindow you expected. For your use case, you should probably just notify the stream listener to null out the reference inside `MediaDocument::Destroy`.

Comment 7

[Benjamin Chen [:bechen]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204490/diff/2-3/

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

https://reviewboard.mozilla.org/r/204490/#review210554

So PluginDocument has https://searchfox.org/mozilla-central/source/dom/html/PluginDocument.cpp#135
Could we do the same thing here? Consistency would reduce the risk for regressions.
Could you please check all these media-like documents.

::: dom/html/VideoDocument.cpp:31
(Diff revision 3)
>                                       nsIStreamListener** aDocListener,
>                                       bool                aReset = true,
>                                       nsIContentSink*     aSink = nullptr);
>    virtual void SetScriptGlobalObject(nsIScriptGlobalObject* aScriptGlobalObject);
>  
> +  virtual void Destroy() {

{ goes to its own line here

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204490/diff/3-4/

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

https://reviewboard.mozilla.org/r/204490/#review210948

I guess we can do this. ImageDocument does still something a bit different and would be nice to have consistent handling also there, but that can be done in a different bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0c952f7d225
Break the mDocument reference cycle at Destroy(). r=jwwang,smaug

Comment 12

[Tiberius Oros[:tiberius_oros]]

Backed out changeset b0c952f7d225 (bug 1421004)for build bustage r=backout

https://hg.mozilla.org/integration/autoland/rev/c408aa5f26f9c0e2a7ffcae82364379a73d9f1da

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b0c952f7d225181a76d4dafbe144dcd72e10b08b

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204490/diff/4-5/

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8933558 [details]
Bug 1421004 - Break the mDocument reference cycle at Destroy().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204490/diff/5-6/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0aeba8f81a2
Break the mDocument reference cycle at Destroy(). r=jwwang,smaug

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c0aeba8f81a2