Split up leak detector into two stages for better leak reporting.

Split up leak detector into two stages for better leak reporting.

The leak detector clears out resources along with issuing a sequence of GCs
before taking object census. It then counting up resources that are left and
reporting these as leaking.

With Oilpan enabled, RenderViewTest needs to carefully orchestrate its shutdown
to reliably not report the frame(s) attached to the view as leaking. (With Oilpan
enabled, frames will delayed'ly release resources upon frame close()ing requiring
a follow-on GC to clear out those resources.)

Accommodate that by splitting out the leak detector into two -- with RenderViewTest
injecting the clearing of its view in between those.

R=haraken, jochen, hajimehoshi
BUG=561293
Committed: https://crrev.com/e6c4efc01a4e1d50713542460d69d64ae5e76094
Cr-Commit-Position: refs/heads/master@{#361638}

[haraken, 2015-11-24 23:54:50]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-11-24 23:54:51]

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
content/public/test/render_view_test.cc:296:
leak_detector->delayFinalGarbageCollection();

Would you elaborate on why we need to delay the GC?

https://codereview.chromium.org/1472943004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLeakDetector.cpp (right):

https://codereview.chromium.org/1472943004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLeakDetector.cpp:97: void
WebLeakDetectorImpl::collectGarbageAndGetDOMCounts(WebLocalFrame* frame)

Instead of introducing a notion of "delayed", I'd split this method into two
e.g., disposeBlinkMemory & collectGarabge.

https://codereview.chromium.org/1472943004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLeakDetector.cpp:120:
V8GCController::collectAllGarbageForTesting(isolate);

This will run Oilpan's GC. Doesn't it need to be delayed as well?

[sof, 2015-11-25 06:20:50]

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
content/public/test/render_view_test.cc:296:
leak_detector->delayFinalGarbageCollection();
On 2015/11/24 23:54:51, haraken wrote:
> 
> Would you elaborate on why we need to delay the GC?

Clearing view_ below will trigger a close() on the frame _and_ its frame tree,
which will release (self-)Persistent<>s, allowing objects to be swept at the
next GC.

If we don't ensure/guarantee that any of the leak detector's GCs run after that
close() has gone ahead, we will see frame leaks reported.

[haraken, 2015-11-25 07:39:38]

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
content/public/test/render_view_test.cc:296:
leak_detector->delayFinalGarbageCollection();
On 2015/11/25 06:20:50, sof wrote:
> On 2015/11/24 23:54:51, haraken wrote:
> > 
> > Would you elaborate on why we need to delay the GC?
> 
> Clearing view_ below will trigger a close() on the frame _and_ its frame tree,
> which will release (self-)Persistent<>s, allowing objects to be swept at the
> next GC.
> 
> If we don't ensure/guarantee that any of the leak detector's GCs run after
that
> close() has gone ahead, we will see frame leaks reported.

Doesn't it mean just that we need to trigger another GC after clearing view_?
(i.e., don't need to delay the GC; we can just trigger another GC after clearing
view_.)

I'd prefer not introducing a complicated logic like delaying.

[sof, 2015-11-25 07:59:54]

sigbjornf@opera.com changed reviewers:
+ hajimehoshi@chromium.org, jochen@chromium.org

[sof, 2015-11-25 07:59:54]

please take a look.

Addressing reported frame "leaks" from
 

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/...

[sof, 2015-11-25 08:01:10]

On 2015/11/25 07:39:38, haraken wrote:
>
https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
> File content/public/test/render_view_test.cc (right):
> 
>
https://codereview.chromium.org/1472943004/diff/60001/content/public/test/ren...
> content/public/test/render_view_test.cc:296:
> leak_detector->delayFinalGarbageCollection();
> On 2015/11/25 06:20:50, sof wrote:
> > On 2015/11/24 23:54:51, haraken wrote:
> > > 
> > > Would you elaborate on why we need to delay the GC?
> > 
> > Clearing view_ below will trigger a close() on the frame _and_ its frame
tree,
> > which will release (self-)Persistent<>s, allowing objects to be swept at the
> > next GC.
> > 
> > If we don't ensure/guarantee that any of the leak detector's GCs run after
> that
> > close() has gone ahead, we will see frame leaks reported.
> 
> Doesn't it mean just that we need to trigger another GC after clearing view_?
> (i.e., don't need to delay the GC; we can just trigger another GC after
clearing
> view_.)
> 
> I'd prefer not introducing a complicated logic like delaying.

To inject more GCs in the middle like that doesn't take care of it; checked
earlier.

[sof, 2015-11-25 08:01:47]

+Cc: oilpan-reviews

[haraken, 2015-11-25 08:16:30]

Thanks, LGTM with nits.

https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLeakDetector.h (right):

https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLeakDetector.h:76: virtual void
collectGarbageAndGetDOMCounts(WebLocalFrame*) = 0;

There is only one call site of collectGarbageAndGetDOMCounts. I'd remove the
collectGarbageAndGetDOMCounts and replace it with prepareForLeaDetection +
collectGarbageAndReport (to decrease the number of public APIs).

Then m_haveReported won't be needed.

[jochen (gone - plz use gerrit), 2015-11-25 08:35:38]

lgtm

[sof, 2015-11-25 08:40:21]

https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLeakDetector.h (right):

https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLeakDetector.h:76: virtual void
collectGarbageAndGetDOMCounts(WebLocalFrame*) = 0;
On 2015/11/25 08:16:30, haraken wrote:
> 
> There is only one call site of collectGarbageAndGetDOMCounts. I'd remove the
> collectGarbageAndGetDOMCounts and replace it with prepareForLeaDetection +
> collectGarbageAndReport (to decrease the number of public APIs).
> 
> Then m_haveReported won't be needed.

Done, retired collectGarbageAndGetDOMCounts() + synced surrounding comments.

[hajimehoshi, 2015-11-25 08:41:29]

lgtm

[haraken, 2015-11-25 08:41:48]

On 2015/11/25 08:40:21, sof wrote:
>
https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebLeakDetector.h (right):
> 
>
https://codereview.chromium.org/1472943004/diff/80001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebLeakDetector.h:76: virtual void
> collectGarbageAndGetDOMCounts(WebLocalFrame*) = 0;
> On 2015/11/25 08:16:30, haraken wrote:
> > 
> > There is only one call site of collectGarbageAndGetDOMCounts. I'd remove the
> > collectGarbageAndGetDOMCounts and replace it with prepareForLeaDetection +
> > collectGarbageAndReport (to decrease the number of public APIs).
> > 
> > Then m_haveReported won't be needed.
> 
> Done, retired collectGarbageAndGetDOMCounts() + synced surrounding comments.

LGTM

[sof, 2015-11-25 08:50:56]

Description was changed from

==========
Delay completing leak detector until Blink has shut down.

R=
BUG=
==========

to

==========
Split up leak detector into two stages for better leak reporting.

The leak detector clears out resources along with issuing a sequence of GCs
before taking object census. It then counting up resources that are left and
reporting these as leaking.

With Oilpan enabled, RenderViewTest needs to carefully orchestrate its shutdown
to reliably not report the frame(s) attached to the view as leaking. (With
Oilpan
enabled, frames will delayed'ly release resources upon frame close()ing
requiring
a follow-on GC to clear out those resources.)

Accommodate that by splitting out the leak detector into two -- with
RenderViewTest
injecting the clearing of its view in between those.

R=haraken, jochen, hajimehoshi
BUG=561293
==========

[sof, 2015-11-25 08:51:45]

thanks, added a description giving details + filed the tracking bug
http://code.google.com/p/chromium/issues/detail?id=561293

[sof, 2015-11-25 08:52:11]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-11-25 08:52:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943004/#ps100001
(title: "retire collectGarbageAndGetDOMCounts()")

[commit-bot: I haz the power, 2015-11-25 08:52:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1472943004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1472943004/100001

[commit-bot: I haz the power, 2015-11-25 10:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-25 10:10:02]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2015-11-25 10:51:28]

aww, introduced a leak of ScriptRegExpContexts. Adjusting & retesting.

[sof, 2015-11-25 11:45:59]

On 2015/11/25 10:51:28, sof wrote:
> aww, introduced a leak of ScriptRegExpContexts. Adjusting & retesting.

ps#7 addressed that one.

[haraken, 2015-11-25 11:53:40]

LGTM

[sof, 2015-11-25 12:11:22]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-11-25 12:11:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943004/#ps120001
(title: "call clearScriptRegexpContext() in prepareForLeakDetection()")

[commit-bot: I haz the power, 2015-11-25 12:11:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1472943004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1472943004/120001

[commit-bot: I haz the power, 2015-11-25 13:13:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-25 13:14:13]

Patchset 7 (id:??) landed as
https://crrev.com/e6c4efc01a4e1d50713542460d69d64ae5e76094
Cr-Commit-Position: refs/heads/master@{#361638}