1286751 - [webvtt] memory leak in webvtt.

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

STR:
1. open https://jsfiddle.net/5vko6nby/2/ by my local nightly
2. close the browser

==
  https://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
  http://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
[Child 21200] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/benjamin/hg/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 171
[Child 21200] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/benjamin/hg/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 171
nsStringStats
 => mAllocCount:          30054
 => mReallocCount:         2210
 => mFreeCount:           26902  --  LEAKED 3152 !!!
 => mShareCount:          50390
 => mAdoptCount:           2720
 => mAdoptFreeCount:       2720
 => Process ID: 21200, Thread ID: 140608420653760
--DOMWINDOW == 11 (0x7f87f92ab000) [pid = 21149] [serial = 4] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 10 (0x7f87f92aa000) [pid = 21149] [serial = 3] [outer = (nil)] [url = chrome://browser/content/browser.xul]
--DOMWINDOW == 9 (0x7f87f7a30400) [pid = 21149] [serial = 7] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 8 (0x7f87ef769800) [pid = 21149] [serial = 11] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 7 (0x7f87f7a4e000) [pid = 21149] [serial = 6] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 6 (0x7f87ef748000) [pid = 21149] [serial = 10] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 5 (0x7f87e0f64400) [pid = 21149] [serial = 16] [outer = (nil)] [url = https://self-repair.mozilla.org/en-US/repair]
--DOMWINDOW == 4 (0x7f87e0eb4000) [pid = 21149] [serial = 15] [outer = (nil)] [url = data:application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/>]
--DOMWINDOW == 3 (0x7f87e0eaa000) [pid = 21149] [serial = 13] [outer = (nil)] [url = data:application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/>]
--DOMWINDOW == 2 (0x7f87feec1800) [pid = 21149] [serial = 1] [outer = (nil)] [url = resource://gre-resources/hiddenWindow.html]
--DOMWINDOW == 1 (0x7f87e0f6dc00) [pid = 21149] [serial = 18] [outer = (nil)] [url = https://self-repair.mozilla.org/en-US/repair]
--DOMWINDOW == 0 (0x7f87f9129800) [pid = 21149] [serial = 5] [outer = (nil)] [url = resource://gre-resources/hiddenWindow.html]
Leaked URLs:
  http://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
  http://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
  https://jsfiddle.net/5vko6nby/2/
  https://fiddle.jshell.net/5vko6nby/2/show/
  https://fiddle.jshell.net/5vko6nby/2/show/
  https://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
  http://www.iandevlin.com/html5test/webvtt/upc-video-subtitles-en.vtt
nsStringStats
 => mAllocCount:         100696
 => mReallocCount:         6009
 => mFreeCount:          100644  --  LEAKED 52 !!!
 => mShareCount:         104304
 => mAdoptCount:           4767
 => mAdoptFreeCount:       4765  --  LEAKED 2 !!!
 => Process ID: 21149, Thread ID: 140222640957312

Comment 1

[Benjamin Chen [:bechen]]

Attachment: leak log

Comment 2

[Benjamin Chen [:bechen]]

After some investigation, leak comes from 
https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.cpp#221

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Benjamin Chen [:bechen] from comment #2)
> After some investigation, leak comes from 
> https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.
> cpp#221

Could you check whether ReleaseListeners() here was called after OnStopRequest?
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2646

Comment 4

[Benjamin Chen [:bechen]]

(In reply to Shian-Yow Wu [:swu] from comment #3)
> (In reply to Benjamin Chen [:bechen] from comment #2)
> > After some investigation, leak comes from 
> > https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.
> > cpp#221
> 
> Could you check whether ReleaseListeners() here was called after
> OnStopRequest?
> https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpBaseChannel.cpp#2646

The WebVTTListener::OnStartRequest and WebVTTListener::OnStopRequest are never called after AsyncOpen2().
I will check ReleaseListeners() later.

Comment 5

[Benjamin Chen [:bechen]]

(In reply to Benjamin Chen [:bechen] from comment #4)
> (In reply to Shian-Yow Wu [:swu] from comment #3)
> > (In reply to Benjamin Chen [:bechen] from comment #2)
> > > After some investigation, leak comes from 
> > > https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.
> > > cpp#221
> > 
> > Could you check whether ReleaseListeners() here was called after
> > OnStopRequest?
> > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> > HttpBaseChannel.cpp#2646
> 
> The WebVTTListener::OnStartRequest and WebVTTListener::OnStopRequest are
> never called after AsyncOpen2().
> I will check ReleaseListeners() later.

The HttpBaseChannel::ReleaseListeners() doesn't been called either.

Comment 6

[Shian-Yow Wu [:swu]]

Was the channel destroyed after window closed?  If not, there could be a reference cycle(between HTMLTrackElement/WebVTTListener/nsIChannel) which makes the channel been hold.  If that's the case, since the OnStopRequest was not called, someone has to detect the window close event then call nsIChannel.Cancel() to break the cycle first.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: trackleak.patch

There is a cycle: TrackElement --> Channel --> WebVttListener --> TrackElement
 
The patch tries to listen the shutdownobserver at TrackElement then break the cycle, but still leak.

Comment 8

[Benjamin Chen [:bechen]]

(In reply to Benjamin Chen [:bechen] from comment #7)
> Created attachment 8772309 [details] [diff] [review]
> trackleak.patch
> 
> There is a cycle: TrackElement --> Channel --> WebVttListener -->
> TrackElement
>  
> The patch tries to listen the shutdownobserver at TrackElement then break
> the cycle, but still leak.

The patch seems resolve the leak in content process, but still leak in main process. Any idea?

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8772309 [details] [diff] [review]
trackleak.patch

Review of attachment 8772309 [details] [diff] [review]:
-----------------------------------------------------------------

Using shutdown observer doesn't seem to solve the real issue. HTMLTrackElement should be released after window close / reload.
You should use load group to monitor if document is unload, and close channel at that time.
WebSocket is a good example to follow: https://dxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#2654

Comment 10

[Shian-Yow Wu [:swu]]

Thanks for comment from SC.

Comment 11

[Benjamin Chen [:bechen]]

Attachment: Bug 1286751 - Close the channel when window destroyed.

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

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8775415 [details]
Bug 1286751 - Close the channel when window destroyed.

https://reviewboard.mozilla.org/r/67604/#review64836

I suppose another approach is to register with the cycle collector, but it seems not necessary when the cycle is only through C++ objects.

::: dom/html/HTMLTrackElement.cpp:90
(Diff revision 1)
> +  }
> +  void RegisterWindowDestroyObserver()
> +  {
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (os) {
> +      obs->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, false);

`obs` or `os`?

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8775415 [details]
Bug 1286751 - Close the channel when window destroyed.

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

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8775415 [details]
Bug 1286751 - Close the channel when window destroyed.

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

Comment 15

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a64e038dd0

Comment 16

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8b1f5f8104b

Comment 17

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de9b5b3232de

Comment 18

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4929344c87a7

Comment 19

[Benjamin Chen [:bechen]]

Comment on attachment 8775415 [details]
Bug 1286751 - Close the channel when window destroyed.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/095802807754
Close the channel when window destroyed. r=rillian

Comment 21

[Carsten Book [:Tomcat]]

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