Renderer memory leak: IncrementLoadEventDelayCount is not cleared in ImageLoader |
|||
Issue descriptionThe entire document is leaking on renderer over page navigation with https://www.macys.com & https://www.citi.com. Real-world leak detector found this by running on Cluster Telemetry. This is caused because there is a reference cycle between ImageLoader, IncrementLoadEventDelayCount, Document & ImageElement. IncrementLoadEventDelayCount, which holds Document as a Persistent, is not being cleared in ImageLoader when it's supposed to, and that is why Document cannot be cleared either.
,
Oct 11 2017
,
Oct 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c2f4dfb806c0d89e9a50357d1ad94a95141db23b commit c2f4dfb806c0d89e9a50357d1ad94a95141db23b Author: Yuzu Saijo <yuzus@chromium.org> Date: Mon Oct 16 04:12:34 2017 Clear delay_until_do_update_from_element when clearing pending_task This CL intends to fix a renderer memory leak that happens with document.cloneNode(1) when document has an img that causes an load error. Here's what is happening in the leaking case (see the added layout test): (1) Document::cloneNode() is called with deep==true (2) ContainerNode::CloneChildNodes(clone) is called (3) inside CloneChildNodes, clone->AppendChild(img->cloneNode(true), ...) operation happens (4) img->cloneNode(true) is called and on the cloned img AttributeChangedFromParserOrByCloning is called (5) ImageLoader::UpdateFromImage is called on the cloned img (6) Inside UpdateFromImage, ShouldImmediatelyLoad returns false because of loading error, and as a result, EnqueueImageLoadingMicroTask is called (note that the owner document of the cloned img is the original one here and thus active) (7) clone->AppendChild is called & the owner document of the cloned img becomes the cloned document (8) ImageLoader::UpdateFromImage is called (9) Inside UpdateFromImage, ShouldImmediatelyLoad returns false because of loading error, but this time EnqueueImageLoadingMicroTask is not called because the owner document is inactive (cloned one) (10) delay_until_do_update_from_element that is created on the step (6)EnqueueImageLoadingMicroTask ends up not being cleared. This CL clears delay_until_do_update_from_element even when EnqueueImageLoadingMicroTask is not called. This memory leak was happening with 2 websites out of 1000 when I tested with cluster telemetry and the entire document was leaking. Bug: 773571 Test: third_party/WebKit/LayoutTests/loader/image-loader-clone-leak.html Change-Id: Ib41cde77e7414bf1d35ab6e25695d665d6028b1b Reviewed-on: https://chromium-review.googlesource.com/711654 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#508980} [add] https://crrev.com/c2f4dfb806c0d89e9a50357d1ad94a95141db23b/third_party/WebKit/LayoutTests/loader/image-loader-clone-leak.html [modify] https://crrev.com/c2f4dfb806c0d89e9a50357d1ad94a95141db23b/third_party/WebKit/Source/core/loader/ImageLoader.cpp
,
Oct 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/51ad2417ab848793ce9ca38666dc6c01e30be658 commit 51ad2417ab848793ce9ca38666dc6c01e30be658 Author: Yuzu Saijo <yuzus@chromium.org> Date: Mon Oct 16 06:38:08 2017 Change Persistent to WeakPersisitent in IncrementLoadEventDelayCount This CL changes Persisent<Document> in IncrementLoadEventDelayCount to WeakPersistent<Document>. This is because IncrementLoadEventDelayCount should not prolong the life of a document. Bug: 773571 Change-Id: Ibe759181a00a8ec4d42691ec1f3618bbeea4ffe6 Reviewed-on: https://chromium-review.googlesource.com/720640 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Keishi Hattori <keishi@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Cr-Commit-Position: refs/heads/master@{#508990} [modify] https://crrev.com/51ad2417ab848793ce9ca38666dc6c01e30be658/third_party/WebKit/Source/core/dom/IncrementLoadEventDelayCount.h
,
Oct 16 2017
,
Oct 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16b522819cf9048ca16ce0a47cfa95cc8de8e2b5 commit 16b522819cf9048ca16ce0a47cfa95cc8de8e2b5 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Tue Oct 17 22:20:39 2017 Null-check |document_| in IncrementLoadEventDelayCount After https://chromium-review.googlesource.com/720640, |document_| is WeakPersistent and thus it should be null-checked before use. Bug: 773571 , 775090 Change-Id: I914b57071fa07288997162d8645bab4ded440032 Reviewed-on: https://chromium-review.googlesource.com/721681 Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Keishi Hattori <keishi@chromium.org> Cr-Commit-Position: refs/heads/master@{#509567} [modify] https://crrev.com/16b522819cf9048ca16ce0a47cfa95cc8de8e2b5/third_party/WebKit/Source/core/dom/IncrementLoadEventDelayCount.cpp |
|||
►
Sign in to add a comment |
|||
Comment 1 by yuzus@chromium.org
, Oct 11 2017