Potential memory leak in ExtensionWebRequestTimeTracker::LogRequestStartTime. |
||||||
Issue descriptionI've been looking at sources of browser memory bloat by using native heap profiling on my own browser. Full details: https://docs.google.com/document/d/1fN5balfyrd7sRpd6DRaUI1TwoOwYjLyRSd7mwZT5US8/edit# Over the course of 1 week, the browser process created ~28k objects in ExtensionWebRequestTimeTracker::LogRequestStartTime() that it did not destroy. This is suggestive of a large leak. Each screenshot shows: 1) # of objects created [that have not been destroyed] 2) The stack trace of the code that created the object. rdevlin: Can you take a look? I took a quick look at the code, and I think there's at least 1 leak. """ while (request_ids_.size() > kMaxRequestsLogged) { int64_t to_remove = request_ids_.front(); request_ids_.pop(); std::map<int64_t, RequestTimeLog>::iterator iter = request_time_logs_.find(to_remove); if (iter != request_time_logs_.end() && iter->second.completed) { request_time_logs_.erase(iter); moderate_delays_.erase(to_remove); excessive_delays_.erase(to_remove); } } """ When we clear out requests, we don't clear out request_time_logs_ if the request hasn't completed. But when the request finally completes, we still don't clear it out. I don't know if this is the only leak or if there's more.
,
Jun 14 2017
triage -> karandeepb@, mind taking a look?
,
Jun 19 2017
,
Jun 19 2017
For the case Eric mentioned, along with the request logs, when the request actually completes, it may insert the id into the |excessive_delays_| and |moderate_delays_| which would then leak as well. (which should explain c#1). Devlin: ExtensionWebRequestTimeTracker has a lot of book-keeping to support ExtensionWebRequestTimeTrackerDelegate, but it isn't being used currently. The DefaultDelegate's implementation was commented out in https://codereview.chromium.org/8292006. Do we plan to support this? If not, we can simplify ExtensionWebRequestTimeTracker. Aside: The logic for invoking NotifyExcessiveDelays/NotifyModerateDelays also doesn't seem correct (as per its documentation). While request_ids_ does store the last seen requests, this is not true for |excessive_delays_| and |moderate_delays_| as per the current implementation, but this is probably just a bug.
,
Jun 22 2017
Ping Devlin.
,
Jun 22 2017
+cc battre, author of https://codereview.chromium.org/8292006. My instinct is that, if this hasn't been used in 5 years, it's probably due for cleanup. I'm fine with removing this logic and simplifying the code, and if we want it back, it will live on in git history. Note: we *should* make sure to keep UMA metrics.
,
Jun 23 2017
I agree. I have created a quick CL that strips a lot of unnecessary code: https://chromium-review.googlesource.com/c/544951/
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50f286ab8e95cfe3d18e6abe4444cf5de66741fe commit 50f286ab8e95cfe3d18e6abe4444cf5de66741fe Author: Dominic Battre <battre@chromium.org> Date: Wed Jun 28 16:00:35 2017 Trim ExtensionWebRequestTimeTracker and fix memory leaks Bug: 732875 Change-Id: Ie70b04feefdb95f73af4b4f77248bfc75605b1e0 Reviewed-on: https://chromium-review.googlesource.com/544951 Commit-Queue: Dominic Battré <battre@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#483008} [modify] https://crrev.com/50f286ab8e95cfe3d18e6abe4444cf5de66741fe/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/50f286ab8e95cfe3d18e6abe4444cf5de66741fe/extensions/browser/api/web_request/web_request_time_tracker.cc [modify] https://crrev.com/50f286ab8e95cfe3d18e6abe4444cf5de66741fe/extensions/browser/api/web_request/web_request_time_tracker.h [modify] https://crrev.com/50f286ab8e95cfe3d18e6abe4444cf5de66741fe/extensions/browser/api/web_request/web_request_time_tracker_unittest.cc
,
Jun 28 2017
,
Aug 2 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by erikc...@chromium.org
, Jun 13 2017369 KB
369 KB View Download
383 KB
383 KB View Download