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

Issue 769627 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Potential memory leak in ChromeDataUseAscriber

Project Member Reported by erikc...@chromium.org, Sep 28 2017

Issue description

The desktop memory team is profiling the browser process by recording all allocations, and tracking live allocations. Reports are automatically uploaded when memory usage crosses a threshold.

In this case, we received a report of a browser process that was using 310MB of malloc on macOS. go/crash/ffd15e77c368dde7.

I've attached a json file that shows all the major allocations in the browser process. To view the json file, copy its contents to http://jsonviewer.stack.hu/. 

In this case, the stack I'm attaching as a screenshot shows that there are ~3600 live objects created in ChromeDataUseAscriber::CreateNewDataUseRecorder, taking up ~1MB of memory. Glancing through chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc, it looks like objects are added to data_use_recorders_, and conditionally erased under certain state changes. My guess is that there exist state changes that result in a DataUseRecorderEntry never being deleted from data_use_recorders_, causing the container to slowly grow over time.

Please help us better understand what's going on here. Note that ~85% of the bugs we've filed with this tool have resulted in the discovery of real issues, so please don't just dismiss this out of hand. :)

+ some OWNERS
+ ssid, mariakhomenko, dskiba, since I imagine this issue also affects Android
 
Screen Shot 2017-09-28 at 4.44.02 PM.png
523 KB View Download
trace-ffd15e77c368dde7.gz
287 KB Download
The magnitude of the issue is larger than I thought. The total amount of memory is closer to 3MB, as there are other objects with similar signatures.
Screen Shot 2017-09-28 at 4.50.09 PM.png
417 KB View Download
Screen Shot 2017-09-28 at 4.51.22 PM.png
296 KB View Download
Screen Shot 2017-09-28 at 4.51.36 PM.png
312 KB View Download
Screen Shot 2017-09-28 at 4.51.48 PM.png
320 KB View Download
Cc: bengr@chromium.org rajendrant@chromium.org

Comment 3 by bengr@chromium.org, Sep 28 2017

Owner: rajendrant@chromium.org
Status: Assigned (was: Untriaged)
If you navigate a tab and close it before commit, this will leak one ChromeDataUseRecorder by keeping it in the list. There was a DCHECK at shutdown to watch for leaks, but this DCHECK was commented out in https://codereview.chromium.org/2868733002

I have verified this behavior in tip of tree linux debug build.
Thanks for the fast investigation! :)
CL in review. Will land after tryjob fixes.

https://chromium-review.googlesource.com/c/chromium/src/+/692713

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 3 2017

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

commit 61bef1f231c8bb6610bd66d5f1a235951553bc2c
Author: rajendrant <rajendrant@chromium.org>
Date: Tue Oct 03 04:35:53 2017

Handle canceled navigations in ChromeDataUseAscriber

Also enable DCHECKs for some containers to be empty upon destruction.

Bug:  769627 
Change-Id: Iec7c5cf35ad68d2eee3cc709ba420a28bf5caa62
Reviewed-on: https://chromium-review.googlesource.com/692713
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505942}
[modify] https://crrev.com/61bef1f231c8bb6610bd66d5f1a235951553bc2c/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
[modify] https://crrev.com/61bef1f231c8bb6610bd66d5f1a235951553bc2c/chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc
[modify] https://crrev.com/61bef1f231c8bb6610bd66d5f1a235951553bc2c/chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc

The memory leak potentially caused by canceled navigations is fixed by #c7

I am holding on to this bug to add more DCHECKS for some more containers. When those DCHECKS are enabled, some unittest failed since they were creating navigations without main renderframe create events, causing some leaks. 
Awesome, thanks for the follow up!
Is this fixed then?
Is this fixed?
Status: Started (was: Assigned)
Status: Fixed (was: Started)

Sign in to add a comment