New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 680898 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 0
Type: Bug-Regression



Sign in to add a comment

Memory leak when continuously playing video

Reported by syed.hus...@appspace.com, Jan 13 2017

Issue description

UserAgent: Mozilla/5.0 (X11; CrOS x86_64 9000.50.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.53 Safari/537.36
Platform: 9000.50.0 (Official Build) beta-channel panther

Steps to reproduce the problem:
1. Install CRX from here : https://www.dropbox.com/s/5e9l2yei0ts58p1/Appspace-videotest.crx?dl=0
2. Open app, it should play a video showing mountain continuously
3. Open task manager, the memory will climb up gradually until App restart itself

What is the expected behavior?
Memory is at the more or less the same level

What went wrong?
The memory climb up gradually until the App restart itself. In Kiosk mode, it will freeze the App.

Did this work before? Yes 53

Chrome version: 56.0.2924.53  Channel: n/a
OS Version: 9000.50.0
Flash Version: Shockwave Flash 24.0 r0
 

Comment 1 by rch@chromium.org, Jan 13 2017

I wonder if this might be  bug 678768 . Where is the video hosted from?
Providing a heap trace is likely very useful; see https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/heap_profiler.md


The video file is located inside the crx and is referenced in video source using relative URL. 

I'll try to provide the heap profile. 
Additional info, I've managed to replicate it successfully using Asus Chromebox CN60 running the beta firmware 56

Comment 5 by rbyers@chromium.org, Jan 17 2017

Cc: abodenha@chromium.org rbyers@chromium.org
Components: -Blink Internals>Media Platform>Apps
Labels: Needs-Bisect
Any idea if this can be reproduced on a website outside a CRX?  Absent such a repro I'll remove Component:Blink - assume it's a CrOS-app specific issue.  /cc abodenha.  Please re-add Component:Blink if this is found to reproduce on a web page on any chrome platform.

Since this is reported as a regression, our best bet is probably do try to do a bisect.  How long do you typically need to leave the video playing for before the increase in memory is obvious?  If it's a long time then it may be challenging to bisect.
It's not reproducible when running it as a website. The memory does climb up but eventually will go back down.

The increase in memory is pretty much directly when the app starts. If you run the CRX in desktop mode, eventually the app will restart itself - I believe because the app was out of memory. I haven't run this in kiosk mode.
Components: -Internals>Media
since this is not html5 video on Chrome, but repro on an CrOS app. remove chrome media team. 
Hi, is there any progress/update on this? Anything that I can help in order to get this going?
I can confirm the same behavior as the author of this thread. Nonstop video playback eats up memory inside CRX.
Cc: sduraisamy@chromium.org
Cc: xiy...@chromium.org
Owner: st...@chromium.org
steel@, could you find an owner for this?

The app that could repro the problem is also published to cws:
https://chrome.google.com/webstore/detail/appspace/ibdfmgnhfbdhbepdiellpehbkkagildf

Comment 13 by st...@chromium.org, Feb 23 2017

Owner: tbarzic@chromium.org
Cc: tbarzic@chromium.org
Owner: mlamouri@chromium.org
Status: Assigned (was: Unconfirmed)
Seems to be 
https://chromium.googlesource.com/chromium/src/+/2c68be804b1f752a88b2c0e2e7a086945ef0494c

When created, MediaControlsMediaEventListener adds itself as a listener for the document's fullscreenchange event [1], but never gets removed, which means that the event listener remains reachable (and thus lingers on) throughout document's lifetime.

This becomes very noticeable linked in this issue - the app keeps creating video elements, each accompanied by its MediaControlsMediaEventListener - the app plays a video clip in a loop by discarding current video element and replacing it with a newly created one when the video clip reaches the end.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp?rcl=1aa3f1c709ebdc77c75fa90263b983d181af91e1&l=30
Labels: -OS-Chrome OS-All
Status: Started (was: Assigned)
Components: -Platform>Apps Blink>Media
Labels: -Pri-2 -Needs-Bisect ReleaseBlock-Stable M-57 M-58 Pri-0
URGENT - PTAL ASAP.

A friendly reminder that M57 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch by Friday (03/03), 5:00 PM PST in order to make into the desktop Stable final build cut. Thank you!

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label or move to M58.

Thank you.

Project Member

Comment 18 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8daac32c17f494d496fb2ca3768d67d9bd77db5e

commit 8daac32c17f494d496fb2ca3768d67d9bd77db5e
Author: mlamouri <mlamouri@chromium.org>
Date: Wed Mar 01 22:10:03 2017

Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG= 680898 
R=zqzhang@chromium.org,ojan@chromium.org

Review-Url: https://codereview.chromium.org/2725873002
Cr-Commit-Position: refs/heads/master@{#454060}

[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControls.h
[add] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.h
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
[modify] https://crrev.com/8daac32c17f494d496fb2ca3768d67d9bd77db5e/third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h

Labels: Merge-Request-57
Cc: ranjitkan@chromium.org
Rechecked the issue on chrome version 58.0.3028.0 on Windows, MAC (Unsigned Build) and ubuntu OS as per the steps provided. Memory fluctuated between 18200 KB to 19200 KB, but did not observe more spike. Played the app continuously for 20 Minutes. After 20 minutes memory slowly increased to 21344 KB. App neither restarted itself.

Assuming this to be fixed, @ mlamouri: Request you to please confirm if this is expected.

Thanks.!

ranjitkan@, what's the numbers for earlier versions?
@mlamouri: Just rechecked on Dev 58.0.3026.3 & Beta 57.0.2987.88 and observed that on launching the app, memory goes straight away to 56995 KB and within 2 minutes it reached 65233 KB on Windows 10 and MAC 10.12.3

But again did not observed an APP restart.

Thanks.!
App restart would probably happen because the computer went OOM. It would happen if you would run the app for long enough on a resource constrained machine.

Thank you for confirming this is fixed! :)

As soon as I have the green light, I will merge this to M57.
Labels: -Merge-Request-57 Merge-Approved-57
Approving merge to M57 branch 2987 based on comment #22 and #23. Also per chat with  mlamouri@, this is a safe merge. There should be no visible differences to the end user apart from saving memory. Please merge ASAP. Thank you.
Project Member

Comment 25 by sheriffbot@chromium.org, Mar 6 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: TE-Verified-M58 TE-Verified-58.0.3028.0
Adding TE-Verified labels for M58 as per the confirmation in comment#23
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 7 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f

commit e724b2c3898091f20dd9e9c302cb12c86cdf5e5f
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Tue Mar 07 15:10:44 2017

Media: fix memory leak because of the document holding on an EventListener.

MediaControls checks if they are part of a document or not in order to unregister
most of its event listeners when removed from a document to avoid leaking memory
until the document is closed.

The approach taken in this CL is the most "web based" because the controls are
using events. Another approach would be to use internal callbacks (from
ContainerNode) or even have a `addWeakEventListener` method in EventTarget. The
latter might be an interesting solution.

BUG= 680898 
R=zqzhang@chromium.org,ojan@chromium.org

Review-Url: https://codereview.chromium.org/2725873002
Cr-Commit-Position: refs/heads/master@{#454060}
(cherry picked from commit 8daac32c17f494d496fb2ca3768d67d9bd77db5e)

Review-Url: https://codereview.chromium.org/2736853003 .
Cr-Commit-Position: refs/branch-heads/2987@{#783}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControls.h
[add] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.h
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
[modify] https://crrev.com/e724b2c3898091f20dd9e9c302cb12c86cdf5e5f/third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h

Status: Fixed (was: Started)
Cc: hdodda@chromium.org
Labels: TE-Verified-57.0.2987.96 TE-Verified-M57
Verified on Mac os 10.12.2 , ubuntu 14.04 and windows 7 using M57 #57.0.2987.96 and issue is fixed.

Played app for 5 minutes and observed memory was at the same level and didn't increase drastically.

Attached screenshot for reference.

Adding TE-Verified labels.

Thanks!
680898.png
595 KB View Download
I have been watching this with great interest. Can you confirm when this will be ready for public deployment? We have customers with Chromebox/bits and waiting for the M57 release.

Sign in to add a comment