New issue
Advanced search Search tips

Issue 773571 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Renderer memory leak: IncrementLoadEventDelayCount is not cleared in ImageLoader

Project Member Reported by yuzus@chromium.org, Oct 11 2017

Issue description

The 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.

 
macys.png
6.2 KB View Download
citi.png
7.4 KB View Download

Comment 1 by yuzus@chromium.org, Oct 11 2017

Components: Blink

Comment 2 by junov@chromium.org, Oct 11 2017

Components: -Blink Blink>Loader
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by yuzus@chromium.org, Oct 16 2017

Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, 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