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

Issue 595672 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue v8:4853

Blocking:
issue 602214



Sign in to add a comment

Ignition holds references to objects in registers preventing GC from collecting dead objects.

Project Member Reported by mythria@chromium.org, Mar 17 2016

Issue description

In interpreter, sometimes a register holds a reference to an object. These registers are not live but they hold a reference because they are not cleared after the use. Since they hold a refence, GC cannot collect these otherwise dead objects. This causes the following layout tests to fail:

editing/input/text-input-controller-leak-document.html
fast/dom/MutationObserver/weak-callback-gc-crash.html
fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
fast/harness/internals-observe-gc.html
fast/mediastream/RTCPeerConnection-lifetime.html
netinfo/gc-frame-listeners.html
netinfo/gc-unused-listeners.html
storage/indexeddb/cursor-leak.html
 
There are more tests that fail because of the same problem. The following tests do not match the expected output:

fast/dom/NodeList/nodelist-reachable.html
fast/dom/StyleSheet/gc-rule-children-wrappers.html
fast/xpath/xpath-iterator-result-should-mark-its-nodeset.html
fast/xpath/xpath-other-nodeset-result-should-mark-its-nodeset.html
fast/xpath/xpath-snapshot-result-should-mark-its-nodeset.html


Blockedon: 4853
Blocking: 602214
Cc: mythria@chromium.org
Now that we've decided we aren't going to clear dead registers, we should rewrite these tests so that they pass. Mythri, would you be willing to take a look at doing this?
Owner: mythria@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, May 17 2016

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

commit f61e5d398556470fc7c79e0c2ad1aa16613905c2
Author: mythria <mythria@chromium.org>
Date: Tue May 17 08:58:39 2016

Modifies few gc related layout tests to work with ignition.

In ignition dead temporary registers may hold references to objects
preventing GC from collecting them. It is expensive to clear the dead
registers and there is no noticable impact on heap size. This cl
updates some of the layout tests to work with ignition.

BUG= chromium:595672 , v8:4280 
LOG=N

Review-Url: https://codereview.chromium.org/1972943002
Cr-Commit-Position: refs/heads/master@{#394086}

[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/dom/MutationObserver/weak-callback-gc-crash.html
[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-reachable.html
[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-rule-children-wrappers.html
[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/xpath/xpath-iterator-result-should-mark-its-nodeset.html
[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/xpath/xpath-other-nodeset-result-should-mark-its-nodeset.html
[modify] https://crrev.com/f61e5d398556470fc7c79e0c2ad1aa16613905c2/third_party/WebKit/LayoutTests/fast/xpath/xpath-snapshot-result-should-mark-its-nodeset.html

Project Member

Comment 7 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00f22678f26f4548e5b748ab4ae57a996ec41323

commit 00f22678f26f4548e5b748ab4ae57a996ec41323
Author: mythria <mythria@chromium.org>
Date: Tue May 17 15:35:36 2016

Fixes tests that use internals.observeGC to work with Ignition.

In Ignition references to objects may remain live on the function's stack frame.
This may prevent GC from collective otherwise dead objects. The objects passed to
internals.ObserveGC function may remain live on the function's stack preventing
GC from collecting them. To avoid this, parameters are not passed directly to this
function. Instead, an inner function is used to access these objects.
This will avoid having references to object on this function's stack.

BUG= chromium:595672 , v8:4280 

Review-Url: https://codereview.chromium.org/1950613005
Cr-Commit-Position: refs/heads/master@{#394130}

[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/fast/harness/internals-observe-gc.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/netinfo/gc-unused-listeners.html
[modify] https://crrev.com/00f22678f26f4548e5b748ab4ae57a996ec41323/third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html

Project Member

Comment 8 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b498f0c93b3cf231f1f1507dd0a4c513a3fc15a6

commit b498f0c93b3cf231f1f1507dd0a4c513a3fc15a6
Author: mythria <mythria@chromium.org>
Date: Wed May 18 14:14:40 2016

[Interpreter] Removes failure expectation for gc related blink_tests.

Updates blink_tests/TestExpecations by removing failure expectation for
gc related tests. These tests are modified to work with ignition by
the following cls:
https://codereview.chromium.org/1972943002/
https://codereview.chromium.org/1950613005/

BUG= v8:4280 , chromium:595672 
LOG=N

Review-Url: https://codereview.chromium.org/1985673002
Cr-Commit-Position: refs/heads/master@{#36326}

[modify] https://crrev.com/b498f0c93b3cf231f1f1507dd0a4c513a3fc15a6/tools/blink_tests/TestExpectationsIgnition

Status: Fixed (was: Started)
Project Member

Comment 10 by bugdroid1@chromium.org, May 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26d9b018a9e55b565815bb21a1dcde75c1b7ac12

commit 26d9b018a9e55b565815bb21a1dcde75c1b7ac12
Author: mythria <mythria@chromium.org>
Date: Mon May 23 15:21:52 2016

Modify some gc related layout tests to work with ignition.

In Ignition, temporary expressions could be live on function's stack frame.
This could prevent GC from collecting the objects referenced by these
expressions till the end of function. It is safer to do all the initialization
work in an inner function when testing gc with Ignition. This cl fixes some
of the tests to do initialization in an inner function. The earlier cl
(https://codereview.chromium.org/1950613005/) did not have the correct fix.

BUG= chromium:595672 
LOG=N

Review-Url: https://codereview.chromium.org/1999013002
Cr-Commit-Position: refs/heads/master@{#395327}

[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/editing/input/text-input-controller-leak-document.html
[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/fast/harness/internals-observe-gc.html
[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/netinfo/gc-frame-listeners.html
[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/netinfo/gc-unused-listeners.html
[modify] https://crrev.com/26d9b018a9e55b565815bb21a1dcde75c1b7ac12/third_party/WebKit/LayoutTests/storage/indexeddb/cursor-leak.html

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2304169a1b293adf38af7f08f325e3d546a06e0c

commit 2304169a1b293adf38af7f08f325e3d546a06e0c
Author: leszeks <leszeks@chromium.org>
Date: Tue Apr 11 15:13:25 2017

Modify some more GC layout tests to work with Ignition

In Ignition, dead temporary registers holding call arguments may still
be holding on to old values which should have been GCed. Perform these
calls inside function closures so that their stacks (and these temporary
registers) are cleaned up before calling gc().

This was previously fixed in https://codereview.chromium.org/1972943002
and https://codereview.chromium.org/1950613005, but appeared again when
Ignition added implicit undefined registers and started overwriting
fewer registers in global calls.

BUG= chromium:595672 

Review-Url: https://codereview.chromium.org/2813873002
Cr-Commit-Position: refs/heads/master@{#463636}

[modify] https://crrev.com/2304169a1b293adf38af7f08f325e3d546a06e0c/third_party/WebKit/LayoutTests/netinfo/gc-unused-listeners.html
[modify] https://crrev.com/2304169a1b293adf38af7f08f325e3d546a06e0c/third_party/WebKit/LayoutTests/storage/indexeddb/cursor-request-cycle.html
[modify] https://crrev.com/2304169a1b293adf38af7f08f325e3d546a06e0c/third_party/WebKit/LayoutTests/storage/indexeddb/key-cursor-request-cycle.html

Cc: chandanreddy@google.com cbruni@chromium.org
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 8

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

commit bd6a3aa26dd026720c7ea3e2f19bbb8706f5d3b1
Author: Camillo Bruni <cbruni@chromium.org>
Date: Wed Aug 08 15:43:28 2018

Fix layout test to work with latest ignition changes

Use IIFEs to limit potential leaks via the stack frame to trigger the
gc callbacks.

Bug:  chromium:595672 
Change-Id: I2565d5f7a46faf2e27618bc5b94c96365fd47584
Reviewed-on: https://chromium-review.googlesource.com/1167188
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581572}
[modify] https://crrev.com/bd6a3aa26dd026720c7ea3e2f19bbb8706f5d3b1/third_party/WebKit/LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html

Sign in to add a comment