http/tests/notifications/serviceworker-notification-properties.html leaking |
||||||||||
Issue descriptionFirst 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?)
,
Jun 3 2016
No idea about the culprit, so will disable the tests.
,
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
,
Jun 3 2016
sigbjornf: Could it be https://codereview.chromium.org/2026993004/ ? It landed soon before that build.
,
Jun 3 2016
I don't see how performing yet another GC before leak detection could start making a test leak.
,
Jun 3 2016
Verified to (also) leak with r397405 reverted.
,
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
,
Jun 3 2016
Might help narrowing it down, not sure, but the leak is introduced by accessing notifications[0].data during step 4 in that test.
,
Jun 6 2016
sigbjornf: thanks for looking! Bisecting. This broke somewhere between 425e3fe4d120e37f6c45e1b38f62cb2f1048a4ff {#397503} and 47379284a4a83df80f1f539e87a069164c558d2e {#397509}
,
Jun 6 2016
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
,
Jun 6 2016
ftr, the leak goes away if Notification::stop() clears the m_developerData ScriptValue, but understanding why it (now) introduces a Document leak, is key.
,
Jun 6 2016
,
Jun 7 2016
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?
,
Jun 7 2016
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
,
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
,
Jun 7 2016
,
Jun 7 2016
,
Jun 8 2016
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.
,
Jan 24 2018
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by vabr@chromium.org
, Jun 3 2016