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

Issue 617137 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

http/tests/notifications/serviceworker-notification-properties.html leaking

Project Member Reported by vabr@chromium.org, Jun 3 2016

Issue description

First failing build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/builds/20086

Error (logs attached):
14:39:59.879 19257 worker/1 http/tests/notifications/serviceworker-notification-properties.html leaked
14:39:59.881 19191 [3893/40879] http/tests/notifications/serviceworker-notification-properties.html failed unexpectedly (leak detected: ({"numberOfLiveActiveDOMObjects":[2,9],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,24],"numberOfLiveResources":[0,4]}))
14:39:59.879 19257 worker/1 http/tests/notifications/serviceworker-notification-properties.html failed:
14:39:59.879 19257 worker/1  leak detected: ({"numberOfLiveActiveDOMObjects":[2,9],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,24],"numberOfLiveResources":[0,4]})

The blamelist is r344733, r344734, r344735, r344736, r344737, r344738, but none looks relevant. (Maybe r344738 which is a V8 roll?)
 
Log File contents.html
7.8 MB View Download

Comment 1 by vabr@chromium.org, Jun 3 2016

Argh, ignore the blamelist, the correct one is:
r397504, r397505, r397506, r397507, r397508, r397509.

r397509 is the V8 roll.

Comment 2 by vabr@chromium.org, Jun 3 2016

Components: Blink>ServiceWorker Tests>Disabled
No idea about the culprit, so will disable the tests.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 3 2016

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

commit 6b4478c2f9878f15543c62119e39f1e4b424a156
Author: vabr <vabr@chromium.org>
Date: Fri Jun 03 14:33:48 2016

Disable http/tests/notifications/serviceworker-notification-properties.html on Linux

It has a leak reported. More info on the bug.

TBR=jbroman@chromium.org
NOTRY=true
BUG= 617137 

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

[modify] https://crrev.com/6b4478c2f9878f15543c62119e39f1e4b424a156/third_party/WebKit/LayoutTests/TestExpectations

Cc: vabr@chromium.org
Components: Blink>PushAPI
Owner: sigbjo...@opera.com
sigbjornf: Could it be https://codereview.chromium.org/2026993004/ ?
It landed soon before that build.
I don't see how performing yet another GC before leak detection could start making a test leak.
Cc: falken@chromium.org sigbjo...@opera.com
Owner: ----
Status: Available (was: Assigned)
Verified to (also) leak with r397405 reverted.
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 3 2016

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

commit 137654a60d37788e5206bd62fb074ae4af04b392
Author: jbroman <jbroman@chromium.org>
Date: Fri Jun 03 15:50:53 2016

Move expectation for serviceworker-notification-properties.html leak to LeakExpectations.

The current expectation is for flaky failure, but a leak was observed.

BUG= 617137 
TBR=vabr@chromium.org
NOTRY=true

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

[modify] https://crrev.com/137654a60d37788e5206bd62fb074ae4af04b392/third_party/WebKit/LayoutTests/LeakExpectations
[modify] https://crrev.com/137654a60d37788e5206bd62fb074ae4af04b392/third_party/WebKit/LayoutTests/TestExpectations

Might help narrowing it down, not sure, but the leak is introduced by accessing notifications[0].data during step 4 in that test.
sigbjornf: thanks for looking!

Bisecting. This broke somewhere between 425e3fe4d120e37f6c45e1b38f62cb2f1048a4ff {#397503} and 47379284a4a83df80f1f539e87a069164c558d2e {#397509}


Cc: kouhei@chromium.org hablich@chromium.org
Components: Blink>JavaScript Blink>MemoryAllocator>GarbageCollection
Owner: gsat...@chromium.org
Status: Assigned (was: Available)
I confirmed that reverting the V8 roll fixes the leak:
https://chromium.googlesource.com/chromium/src/+/47379284a4a83df80f1f539e87a069164c558d2e

This change is the likely culprit:
df4f8a2 Promises: Make PromiseSet operation monomorphic by gsathya
https://chromium.googlesource.com/v8/v8/+/df4f8a2b9ee9e474e674301718d19b63650a0ba5

gsathya: Could you take a look? I'm not sure if the underlying cause would be in V8 or Blink's gc.

Here is how to run the leak detector:
$ ninja -j 400 -C out/Default blink_tests
$ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default --no-retry-failures --enable-leak-detection http/tests/notifications/serviceworker-notification-properties.html


ftr, the leak goes away if Notification::stop() clears the m_developerData ScriptValue, but understanding why it (now) introduces a Document leak, is key.
Cc: mlippautz@chromium.org
I ended up reverting df4f8a2:
https://codereview.chromium.org/2047553002/

df4f8a2 had removed resetting of certain promise objects -- these should've been GC'd away anyway, so I'm not sure why this would leak. But it's better to not leave stale objects anyway, so I've reverted this patch.

Surprisingly, while trying to debug this leak, I commented out this line --
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html?q=serviceworker-noti+file:%5Esrc/&sq=package:chromium&l=96

and the leak went away(without my reverting my patch). Also, inlining the |assert_object_is_superset| function, makes the leak go away(also, without reverting my patch). I'm not entirely sure why? 
Thanks gsathya. I will remove the leak expectation since the revert is included in the V8 roll https://chromium.googlesource.com/v8/v8/+log/7ed4b5d5..a4540654
Project Member

Comment 15 by bugdroid1@chromium.org, Jun 7 2016

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

commit d72950f4d4bbaaaa7e8fac03f760eb2d445a2efc
Author: falken <falken@chromium.org>
Date: Tue Jun 07 09:30:33 2016

serviceworker-notification-properties.html no longer leaks

Fixed by a revert in V8:
https://chromium.googlesource.com/v8/v8/+/9b606523b5e210f60c4260ee5f50a54e8f07adbd

BUG= 617137 
TBR=jbroman
NOTRY=true

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

[modify] https://crrev.com/d72950f4d4bbaaaa7e8fac03f760eb2d445a2efc/third_party/WebKit/LayoutTests/LeakExpectations

Comment 16 by vabr@chromium.org, Jun 7 2016

Cc: -vabr@chromium.org
Cc: gsat...@chromium.org
Owner: falken@chromium.org
Status: Fixed (was: Assigned)
Marking fixed since the leak is gone.

Follow-up work to understand the leak and relanding the V8 change without leaking could be tracked in an other issue or by reusing one, but I wouldn't be the owner of that.
Components: Tests>Disabled
Labels: Test-Disabled

Sign in to add a comment