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

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.

I added the URL to the local lab computer. Let's see if this can be reproduced.

Could be related to https://code.google.com/p/chromium/issues/detail?id=530064

memory-internals page after one hour of running this page in the background. Something is wrong.

	result.png 98.7 KB   View   Download

Oh the results are for Canary (47)

I can reproduce, will try to 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.

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

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

Down to 13 revisions: 330765-330778

https://chromium.googlesource.com/chromium/src/+log/c1a1691c5f2547655bb527f18df64e86615f509f..0dccfef67664a8304772e46696be7f15ee177419

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

(No comment was entered for this change.)

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

that's nothing that's going to be merged back to M45.

Please fix the leak in your CL

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?

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.

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?

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

It's not a ton of custom bindings.

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

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?

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?

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?

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.

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?

(No comment was entered for this change.)

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

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

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

Test Tean had confirmed this issue in #8, hence removing the label.

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

bashi@ has a CL up for this

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

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

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

(No comment was entered for this change.)