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

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment
link

Issue 529941: Refreshing iframe causes memory leak and eventual OOM

Reported by rsch...@chromium.org, Sep 9 2015 Project Member

Issue description

Version 45.0.2454.78 (Official Build) beta (64-bit)
URL: http://ryanschoen.com/chromestatus/

I have a Chromebox connected to a monitor in MTV, and it displays the above URL. It cycles between chromestatus.com, the main waterfall, and the perf waterfall. Recently it's been OOM crashing a few times a day. To investigate, I reloaded the page and came back ~30 minutes later. It had grown from 50KB to 500KB.

I'm now seeing this on Linux as well, version 46.0.2490.13. Started around 50KB, a few minutes later we're at 150KB and it seems to keep climbing on each refresh. The pattern is iframe navigation -> huge spike -> ~10s later down to a lower level (but still above the original).

OOM crash IDs:
510330b5db1a8e92
cf009ecdc67ba1bd
b9ff525aa3a067b8
95e1394b00a7087d
03e8559cf6d2a563
4900575e6276ae6b
98f9938bbe76edd1

The first crash report of this type is 0fea067a50fda148, which corresponds to version 45.0.2454.26. So we *might* be looking for something in this range:

https://chromium.googlesource.com/chromium/src/+log/45.0.2454.15..45.0.2454.26?pretty=fuller&n=10000

Though there's no guarantee that the monitor is on a regular update schedule, no one hardly ever touches it. It could have upgraded several versions at once.
 

Comment 1 by rsch...@chromium.org, Sep 9 2015

Whoops, every time I said KB I meant MB. I *wish* we had only grown to 500KB.

Comment 2 by rsch...@chromium.org, Sep 9 2015

Labels: Needs-Bisect
QA, can we get a bisect on this? Based on my reasoning in the original post it may be between 45.0.2454.15 and 45.0.2454.26, although it may be earlier. It's definitely present in 45.0.2454.26.

Comment 3 by hablich@chromium.org, Sep 10 2015

Labels: Needs-TestConfirmation
I added the URL to the local lab computer. Let's see if this can be reproduced.

Comment 5 by hablich@chromium.org, Sep 10 2015

memory-internals page after one hour of running this page in the background. Something is wrong.
result.png
98.7 KB View Download

Comment 6 by hablich@chromium.org, Sep 10 2015

Oh the results are for Canary (47)

Comment 7 by u...@chromium.org, Sep 10 2015

Owner: u...@chromium.org
Status: Assigned
I can reproduce, will try to bisect.

Comment 8 by smokana@chromium.org, Sep 10 2015

Labels: -Needs-Bisect
Able to reproduce the crash on MAC and Chrome: 47.0.2503.0 

Crash ID: 8cb2c60423b30e11 (Out of Memory). However, unable to bisect as this crash is not consistently reproducible. Tried on M45, but crash is not seen.

Comment 9 by u...@chromium.org, Sep 10 2015

45.0.2454.0 is a bad revision (memory goes to 300MB after hour). I didn't find a good revision yet.

Comment 10 by u...@chromium.org, Sep 11 2015

Overnight run narrowed the range down to 45.0.2407.0 - 45.0.2408.0. Bisect continues.

Comment 12 by u...@chromium.org, Sep 14 2015

Cc: haraken@chromium.org
Owner: bashi@chromium.org
Bisected to https://codereview.chromium.org/1130763006, assigning to bashi@chromium.org.

Comment 13 by hablich@chromium.org, Sep 15 2015

Labels: -Cr-Blink-JavaScript-GC Cr-Blink-Bindings

Comment 14 by bashi@chromium.org, Sep 15 2015

Cc: bashi@chromium.org
Owner: jochen@chromium.org
This would be solved by haraken's proposal[1] and I heard that jochen@ is taking over the task.

[1] https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP68/edit?pli=1

Comment 15 by jochen@chromium.org, Sep 16 2015

Cc: jochen@chromium.org
Owner: bashi@chromium.org
that's nothing that's going to be merged back to M45.

Please fix the leak in your CL

Comment 16 by haraken@chromium.org, Sep 16 2015

Our plan was to fix the leak by introducing traceWrapper (including other leaks such as https://code.google.com/p/chromium/issues/detail?id=501866), but you said you want to implement it.

It would be possible to fix the leak without using traceWrapper (using SetWrapperReference & custom visitDOMWrapper etc), but the fix will be as complex as traceWrapper. Also the fix will be replaced with traceWrapper in the near future.

What is your proposal?

Comment 17 by jochen@chromium.org, Sep 16 2015

my proposal is to use the existing means we have to not leak memory (as you described). The fact that those will be replaced shouldn't keep us from fixing such bugs now.

Also, as I said above, we won't merge back a new tracing infrastructure in any case, the bug however affects stable users.

Comment 18 by haraken@chromium.org, Sep 16 2015

As discussed in https://code.google.com/p/chromium/issues/detail?id=501866, I cannot come up with an easy fix mergeable to the old branch. traceWrapper seems to be a more straightforward fix for this to me...

Are you really proposing to fix the leak by adding a ton of custom bindings?

Comment 19 by bashi@chromium.org, Sep 16 2015

Anyway, I'll start woring on this next week.

Comment 20 by jochen@chromium.org, Sep 16 2015

It's not a ton of custom bindings.

you can also punt on this if you feel it's not worth it.

Comment 21 by haraken@chromium.org, Sep 16 2015

I guess we'll need to implement something similar to PromiseRejectionEvent but in a more complicated way because CustomEvent::m_detail needs to support a cross-world ScriptValue (i.e., https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp&q=v8customeventcustom.cpp&sq=package:chromium&type=cs&l=53). I guess the fix is going to be too complicated to merge into an existing branch. (I agree that introducing traceWrapper is complicated as well, but it seems to be a more straightforward way to fix it.)

> you can also punt on this if you feel it's not worth it.

Yeah, although the option is the last resort.

bashi@: Would there be anyway to fix the leak in short term by partially reverting https://codereview.chromium.org/1130763006 somehow?

Comment 22 by rsch...@chromium.org, Sep 17 2015

I have general concerns about there being a memory leak in stable that we cannot fix.

Do we have any idea how general this is? Did my example tickle an extremely small use case? Or could this be rampant?

Comment 23 by haraken@chromium.org, Sep 17 2015

The situation is as follows:

- We have similar leaks in other places as well (e.g., https://code.google.com/p/chromium/issues/detail?id=501866).

- My assumption was that the leak happens only in artificial scenarios. However, the assumption was wrong -- the leak is happening in the real scenario rschoen@ described. I'm not sure how general the leak is.

- It's hard (for me) to come up with a fix mergable to old branches. I think the best thing we can do is to fix the leaks by the next branch cut.

jochen@: Shall we implement traceWrapper and fix the leaks we're aware of (before considering traceWrapper in the context of incremental marking)? It's still not clear when we can really ship & stabilize Oilpan (and thus unblock the incremental marking), so it seems better to implement traceWrapper and fix these leaks by the next branch cut. What do you think?

Comment 24 by bashi@chromium.org, Sep 17 2015

Re: #21

Unfortunately I don't think it's easy. I removed [InitializedByEventConstructor] in https://codereview.chromium.org/1154943009 so if we want to revert https://codereview.chromium.org/1130763006 (even partially), we need to re-implement the feature [InitializedByEventConstructor] provided.

Comment 25 by haraken@chromium.org, Sep 17 2015

Thanks for the investigation!

So the most constructive action I can come up with is to introduce traceWrapper and fix the leaks by the next branch cut.

jochen@: Do you have a bandwidth to implement it? Or do you have other suggestions?

Comment 26 by haraken@chromium.org, Sep 21 2015

Owner: jochen@chromium.org

Comment 27 by haraken@chromium.org, Sep 24 2015

Maybe the leak discussed in https://github.com/Polymer/polymer/issues/2452 is related to this bug.

Comment 28 by haraken@chromium.org, Sep 24 2015

Labels: -Pri-2 Pri-1
I noticed that this is causing a lot of leaks in Polymer apps. Raising the priority to P1.

I'll re-think about a way to fix the leak in short term (even though the fix won't be mergeable to old branches).

Comment 29 by haraken@chromium.org, Sep 24 2015

jochen@: I'm considering to take a similar approach to the PromiseRejectionEvent, but would you help me understand how PromiseRejectionEvent::m_promise and PromiseRejectionEvent::m_reason are kept alive over a minor GC?

m_promise and m_reason are traced in visitDOMWrapper, but visitDOMWrapper is not called in a minor GC at all. (Even if we call visitDOMWrapper in a minor GC, it is still problematic because visitDOMWrapper of wrappers that reside in the old space is not called...)

Comment 30 by ligim...@chromium.org, Sep 24 2015

Labels: -Needs-TestConfirmation
Test Tean had confirmed this issue in #8, hence removing the label.

Comment 31 by haraken@chromium.org, Sep 25 2015

bashi@ is now looking at if we can work around the issue using V8 hidden values.

Comment 32 by jochen@chromium.org, Sep 28 2015

Owner: bashi@chromium.org
bashi@ has a CL up for this

Comment 33 by bugdroid1@chromium.org, Sep 29 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87

commit bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87
Author: bashi <bashi@chromium.org>
Date: Tue Sep 29 02:27:47 2015

Store |detail| as a hidden value of custom event wrappers

We cannot hold strong references from Blink to V8. This means
that we shouldn't use ScriptValue in DOM impl objects.
To stop using ScriptValue in CustomEvent, store |detail|
as a hidden value of custom event wrappers and return it when
the getter of |detail| is called.

BUG= 529941 

Review URL: https://codereview.chromium.org/1372513002

Cr-Commit-Position: refs/heads/master@{#351242}

[add] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak-expected.txt
[add] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak.html
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/LayoutTests/fast/js/constructor-length.html
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/Source/core/events/CustomEvent.cpp
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/Source/core/events/CustomEvent.h
[modify] http://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87/third_party/WebKit/Source/core/events/CustomEvent.idl

Comment 34 by bugdroid1@chromium.org, Oct 1 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2

commit 1cf9a35fef65ba7124b3a4520c2bf33891e18fc2
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Thu Oct 01 18:42:11 2015

Revert of Store |detail| as a hidden value of custom event wrappers (patchset #4 id:60001 of https://codereview.chromium.org/1372513002/ )

Reason for revert:
It caused release blocking  bug 537567 .

BUG= 537567 

Original issue's description:
> Store |detail| as a hidden value of custom event wrappers
>
> We cannot hold strong references from Blink to V8. This means
> that we shouldn't use ScriptValue in DOM impl objects.
> To stop using ScriptValue in CustomEvent, store |detail|
> as a hidden value of custom event wrappers and return it when
> the getter of |detail| is called.
>
> BUG= 529941 
>
> Committed: https://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87
> Cr-Commit-Position: refs/heads/master@{#351242}

TBR=jochen@chromium.org,haraken@chromium.org,bashi@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 529941 

Review URL: https://codereview.chromium.org/1383603004

Cr-Commit-Position: refs/heads/master@{#351851}

[delete] http://crrev.com/8acf48f8eee79de78c4d6aac59ba3ef06daa8918/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak-expected.txt
[delete] http://crrev.com/8acf48f8eee79de78c4d6aac59ba3ef06daa8918/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak.html
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/LayoutTests/fast/js/constructor-length.html
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/Source/core/events/CustomEvent.cpp
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/Source/core/events/CustomEvent.h
[modify] http://crrev.com/1cf9a35fef65ba7124b3a4520c2bf33891e18fc2/third_party/WebKit/Source/core/events/CustomEvent.idl

Comment 35 by bugdroid1@chromium.org, Oct 6 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851

commit ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851
Author: bashi <bashi@chromium.org>
Date: Tue Oct 06 00:41:25 2015

Reland: Store |detail| as a hidden value of custom event wrappers

The original CL was reverted because it broke google image search.
This CL stores |detail| as a hidden value when initCustomEvent() is called
instead of serializing it.

Original description:

We cannot hold strong references from Blink to V8. This means
that we shouldn't use ScriptValue in DOM impl objects.
To stop using ScriptValue in CustomEvent, store |detail|
as a hidden value of custom event wrappers and return it when
the getter of |detail| is called.

BUG= 529941 

Review URL: https://codereview.chromium.org/1383543004

Cr-Commit-Position: refs/heads/master@{#352490}

[add] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak-expected.txt
[add] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/LayoutTests/fast/events/custom-event-detail-leak.html
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/LayoutTests/fast/js/constructor-length.html
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/Source/core/events/CustomEvent.cpp
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/Source/core/events/CustomEvent.h
[modify] http://crrev.com/ce8a2dfe58ba9c8a9fe999124a5a7eaabf1e0851/third_party/WebKit/Source/core/events/CustomEvent.idl

Comment 36 by bashi@chromium.org, Oct 27 2015

Status: Fixed

Comment 37 by dgro...@chromium.org, Jan 31 2017

Looks like this resurfaced a few milestones later. Two reports from the wild:  issue 609137  and  issue 686741 . Could someone knowledgeable take a look?

Comment 38 by dominicc@chromium.org, Apr 7 2017

Status: Untriaged (was: Fixed)
Reopening this per comment 37.

Comment 39 by dominicc@chromium.org, Apr 7 2017

Cc: dominicc@chromium.org kochi@chromium.org

Comment 40 by bashi@chromium.org, Apr 11 2017

Owner: haraken@chromium.org
I wasn't able to repro the leak with http://ryanschoen.com/chromestatus/

The cause was a reference cycle between v8 and Blink and I think it was fixed by trace wrapper.  Issue 60917  looks like a different issue.

haraken@: could you confirm and close this if my understanding is correct?

Comment 41 by haraken@chromium.org, Apr 11 2017

Status: Fixed (was: Untriaged)
I think that this was fixed by #c35. (Wrapper tracing hasn't yet changed the hidden value code.)

I don't think the reported issue in #c37 is related to this issue.

Sign in to add a comment