Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(364)

Issue 2026993004: Delay leak reporting until worker in-process proxies have been finalized. (Closed)

Created:
4 years, 6 months ago by sof
Modified:
4 years, 6 months ago
CC:
chromium-reviews, falken, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Delay leak reporting until worker in-process proxies have been finalized. If a document creates a number of workers, terminating these and having their destruction ripple all the way back to the in-process proxy objects isn't immediate. But something that needs to complete before the leak detector can initiate reporting -- an in-process proxy object maintains a strong reference to the document, and would generate a leak if not destructed and its garbage having been collected afterwards. Address the reliability of multi worker shutdown by maintaining a counter of how many in-process proxy objects are still alive and run GCs until it drops to zero. Do that at most two times around. R=haraken,kouhei BUG=589802, 616714 Committed: https://crrev.com/b0206d8c4aa1f5a659169111a722d183fc2e7739 Cr-Commit-Position: refs/heads/master@{#397405}

Patch Set 1 #

Patch Set 2 : assume main thread usage #

Patch Set 3 : use proxy count to decide if another GC round is reqd #

Patch Set 4 : remove dated comment #

Patch Set 5 : drop back to ps#2 approach #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -2 lines) Patch
M third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp View 1 2 3 4 3 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebLeakDetector.cpp View 1 2 3 4 2 chunks +12 lines, -2 lines 0 comments Download

Messages

Total messages: 33 (13 generated)
sof
please take a look.
4 years, 6 months ago (2016-06-01 15:09:12 UTC) #2
haraken
Just help me understand, why does the leak detector runs before the main thread shuts ...
4 years, 6 months ago (2016-06-01 15:21:52 UTC) #3
sof
On 2016/06/01 15:21:52, haraken wrote: > Just help me understand, why does the leak detector ...
4 years, 6 months ago (2016-06-01 15:36:30 UTC) #4
haraken
On 2016/06/01 15:36:30, sof wrote: > On 2016/06/01 15:21:52, haraken wrote: > > Just help ...
4 years, 6 months ago (2016-06-02 01:12:14 UTC) #5
sof
On 2016/06/02 01:12:14, haraken wrote: > On 2016/06/01 15:36:30, sof wrote: > > On 2016/06/01 ...
4 years, 6 months ago (2016-06-02 05:27:14 UTC) #6
haraken
On 2016/06/02 05:27:14, sof wrote: > On 2016/06/02 01:12:14, haraken wrote: > > On 2016/06/01 ...
4 years, 6 months ago (2016-06-02 05:28:00 UTC) #7
kouhei (in TOK)
lgtm
4 years, 6 months ago (2016-06-02 05:29:18 UTC) #8
sof
Hmm, I think we can do better now that we have this proxy count available ...
4 years, 6 months ago (2016-06-02 05:46:22 UTC) #9
sof
On 2016/06/02 05:46:22, sof wrote: > Hmm, I think we can do better now that ...
4 years, 6 months ago (2016-06-02 06:22:01 UTC) #10
haraken
LGTM
4 years, 6 months ago (2016-06-02 06:27:40 UTC) #11
kouhei (in TOK)
still lgtm
4 years, 6 months ago (2016-06-02 06:28:11 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026993004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026993004/60001
4 years, 6 months ago (2016-06-02 06:41:14 UTC) #15
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 6 months ago (2016-06-02 07:59:47 UTC) #17
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/19895c52589f189d6fdc563759c9872af2cbf86c Cr-Commit-Position: refs/heads/master@{#397333}
4 years, 6 months ago (2016-06-02 08:01:42 UTC) #19
vabr (Chromium)
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2030883003/ by vabr@chromium.org. ...
4 years, 6 months ago (2016-06-02 10:19:15 UTC) #20
sof
Going back to what ps#2 did, as chasing after reported leaks in unit tests that ...
4 years, 6 months ago (2016-06-02 13:17:18 UTC) #22
haraken
LGTM
4 years, 6 months ago (2016-06-02 13:20:46 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026993004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026993004/80001
4 years, 6 months ago (2016-06-02 13:35:34 UTC) #29
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 6 months ago (2016-06-02 15:09:34 UTC) #31
commit-bot: I haz the power
4 years, 6 months ago (2016-06-02 15:10:42 UTC) #33
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/b0206d8c4aa1f5a659169111a722d183fc2e7739
Cr-Commit-Position: refs/heads/master@{#397405}

Powered by Google App Engine
This is Rietveld 408576698