Memory leak in ExtensionAPIFrameIDMap |
||||
Issue descriptionExtensionAPIFrameIDMap is a singleton which is never destroyed i.e. leaked. It seems that we can store FrameData for some render frames as part of the frame_data_map_ in ExtensionAPIFrameIDMap for whom RenderFrameDeleted is not called subsequently, and hence for whom the FrameData entries are never cleared. An example cases I found: 1) TabManagerTest.AutoDiscardable -> Here UpdateTabAndWindowId is called for a RenderFrameHost which is not yet alive and which never becomes alive. We should ensure that we only store FrameData for alive frames. See test failures in https://chromium-review.googlesource.com/c/chromium/src/+/940443 which shows that even after browser shutdown in browser tests (i.e. even after all web contents and hence RenderFrameHosts have been destroyed), we still have some remaining entries in frame_id_map_.
,
Mar 2 2018
,
Mar 2 2018
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/613ba3311438407d2356193b8881759f0125dced commit 613ba3311438407d2356193b8881759f0125dced Author: Karan Bhatia <karandeepb@chromium.org> Date: Fri Mar 09 00:13:57 2018 Extensions: Fix memory leak in ExtensionApiFrameIdMap. Currently in ExtensionApiFrameIdMap::UpdateTabAndWindowId and ExtensionApiFrameIdMap::LookupFrameDataOnUI we don't check whether a RenderFrameHost is alive, before storing it's frame data. It's possible that such a RenderFrameHost never becomes alive, and hence we don't get any OnRenderFrameDeleted notification for it. Since ExtensionApiFrameIdMap is a singleton, this causes a memory leak. To fix, ensure that only live RenderFrameHosts are tracked. Also ensure in InProcessBrowserTest::TearDown that ExtensionApiFrameIdMap is not leaking memory for any browser test. BUG= 817205 Change-Id: I78bf06cee7e10465f9eb5973ee8e59f525a74d93 Reviewed-on: https://chromium-review.googlesource.com/951975 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#541950} [modify] https://crrev.com/613ba3311438407d2356193b8881759f0125dced/chrome/test/BUILD.gn [modify] https://crrev.com/613ba3311438407d2356193b8881759f0125dced/chrome/test/base/in_process_browser_test.cc [modify] https://crrev.com/613ba3311438407d2356193b8881759f0125dced/extensions/browser/extension_api_frame_id_map.cc [modify] https://crrev.com/613ba3311438407d2356193b8881759f0125dced/extensions/browser/extension_api_frame_id_map.h
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6 commit a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6 Author: Ryan Landay <rlanday@chromium.org> Date: Fri Mar 09 00:22:52 2018 Revert "Extensions: Fix memory leak in ExtensionApiFrameIdMap." This reverts commit 613ba3311438407d2356193b8881759f0125dced. Reason for revert: Causing build failures: https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2Fmac_chromium_rel_ng%2F668246%2F%2B%2Frecipes%2Fsteps%2Fanalyze%2F0%2Fstdout /b/c/b/mac/src/buildtools/mac/gn gen //out/Release --check -> returned 1 ERROR at //chrome/test/BUILD.gn:4405:7: Can't load input file. "//extensions/features", ^---------------------- Original change's description: > Extensions: Fix memory leak in ExtensionApiFrameIdMap. > > Currently in ExtensionApiFrameIdMap::UpdateTabAndWindowId and > ExtensionApiFrameIdMap::LookupFrameDataOnUI we don't check whether a > RenderFrameHost is alive, before storing it's frame data. It's possible that > such a RenderFrameHost never becomes alive, and hence we don't get any > OnRenderFrameDeleted notification for it. Since ExtensionApiFrameIdMap is a > singleton, this causes a memory leak. To fix, ensure that only live > RenderFrameHosts are tracked. > > Also ensure in InProcessBrowserTest::TearDown that ExtensionApiFrameIdMap is not > leaking memory for any browser test. > > BUG= 817205 > > Change-Id: I78bf06cee7e10465f9eb5973ee8e59f525a74d93 > Reviewed-on: https://chromium-review.googlesource.com/951975 > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > Commit-Queue: Karan Bhatia <karandeepb@chromium.org> > Cr-Commit-Position: refs/heads/master@{#541950} TBR=sky@chromium.org,rdevlin.cronin@chromium.org,karandeepb@chromium.org Change-Id: Ib200225c2824c865feda8707f081485d3f53d912 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 817205 Reviewed-on: https://chromium-review.googlesource.com/956610 Reviewed-by: Ryan Landay <rlanday@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#541953} [modify] https://crrev.com/a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6/chrome/test/BUILD.gn [modify] https://crrev.com/a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6/chrome/test/base/in_process_browser_test.cc [modify] https://crrev.com/a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6/extensions/browser/extension_api_frame_id_map.cc [modify] https://crrev.com/a0ef3b78286663bbeff7fb3bd3bcc59dfa0440a6/extensions/browser/extension_api_frame_id_map.h
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0330ff2d69610b35c3e191ccec6b56ecb3205c4 commit c0330ff2d69610b35c3e191ccec6b56ecb3205c4 Author: Karan Bhatia <karandeepb@chromium.org> Date: Fri Mar 09 04:39:44 2018 Reland "Extensions: Fix memory leak in ExtensionApiFrameIdMap." This is a reland of 613ba3311438407d2356193b8881759f0125dced. This collided with r541891. Original change's description: > Extensions: Fix memory leak in ExtensionApiFrameIdMap. > > Currently in ExtensionApiFrameIdMap::UpdateTabAndWindowId and > ExtensionApiFrameIdMap::LookupFrameDataOnUI we don't check whether a > RenderFrameHost is alive, before storing it's frame data. It's possible that > such a RenderFrameHost never becomes alive, and hence we don't get any > OnRenderFrameDeleted notification for it. Since ExtensionApiFrameIdMap is a > singleton, this causes a memory leak. To fix, ensure that only live > RenderFrameHosts are tracked. > > Also ensure in InProcessBrowserTest::TearDown that ExtensionApiFrameIdMap is not > leaking memory for any browser test. > > BUG= 817205 > > Change-Id: I78bf06cee7e10465f9eb5973ee8e59f525a74d93 > Reviewed-on: https://chromium-review.googlesource.com/951975 > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > Commit-Queue: Karan Bhatia <karandeepb@chromium.org> > Cr-Commit-Position: refs/heads/master@{#541950} TBR=sky@chromium.org,rdevlin.cronin@chromium.org Bug: 817205 Change-Id: I7b3ac6806d0c747d439e74f80f623a7fd4412050 Reviewed-on: https://chromium-review.googlesource.com/956762 Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#542025} [modify] https://crrev.com/c0330ff2d69610b35c3e191ccec6b56ecb3205c4/chrome/test/BUILD.gn [modify] https://crrev.com/c0330ff2d69610b35c3e191ccec6b56ecb3205c4/chrome/test/base/in_process_browser_test.cc [modify] https://crrev.com/c0330ff2d69610b35c3e191ccec6b56ecb3205c4/extensions/browser/extension_api_frame_id_map.cc [modify] https://crrev.com/c0330ff2d69610b35c3e191ccec6b56ecb3205c4/extensions/browser/extension_api_frame_id_map.h
,
Mar 9 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 Deleted