ScriptContextSetTest should close the main frame before finishing the test

ScriptContextSetTest should close the main frame before finishing the test

ScriptContextSetTest should close the main frame and the webview, and then
collect garbage in Oilpan's heap before finishing the test.
Otherwise, the resources retained by the main frame are reported as leaking
on LSan builds.

BUG=497658
Committed: https://crrev.com/1156707553435bdc1a7830186e38f0e5dc01f8f4
Cr-Commit-Position: refs/heads/master@{#336321}

[haraken, 2015-06-24 11:45:34]

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

[haraken, 2015-06-24 11:45:34]

PTAL

This is blocking us from moving core/fetch/ to Oilpan
(https://codereview.chromium.org/1124153003/).

[haraken, 2015-06-24 11:45:53]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[not at google - send to devlin, 2015-06-24 16:08:47]

lgtm but please see comment and if possible implement it.

also please add a BUG reference.

https://codereview.chromium.org/1210513002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set_unittest.cc (right):

https://codereview.chromium.org/1210513002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set_unittest.cc:67:
blink::WebHeap::collectAllGarbageForTesting();
This will probably regress at some point in the future - or, the next test we
add which needs a frame will fail for similar and mysterious reasons.

If it's important to submit this CL right this second, then ok - but longer term
I'd really like to see a RAAI solution. For example, a file like
"extensions/renderer/scoped_web_frame":

class ScopedWebFrame {
 public:
  ScopedWebFrame() : view_(nullptr), frame_(nullptr) {
    view_ = WebView::Create(nullptr);
    frame_ = WebLocalFrame::create(...);
    view_->setMainFrame(frame);
  }

  ~ScopedWebFrame() {
    view_->close();
    frame_->close();
    WebHeap::collectAllGarbageForTesting();
  }

  WebLocalFrame* frame() { return frame_; }

 private:
  WebView* view_;
  WebLocalFrame* frame_;
};

then other tests can use that as well, *and* we have a canonical way of creating
test frames.

[haraken, 2015-06-25 00:18:38]

Thanks for review!

> also please add a BUG reference.
> 
>
https://codereview.chromium.org/1210513002/diff/1/extensions/renderer/script_...
> File extensions/renderer/script_context_set_unittest.cc (right):
> 
>
https://codereview.chromium.org/1210513002/diff/1/extensions/renderer/script_...
> extensions/renderer/script_context_set_unittest.cc:67:
> blink::WebHeap::collectAllGarbageForTesting();
> This will probably regress at some point in the future - or, the next test we
> add which needs a frame will fail for similar and mysterious reasons.
> 
> If it's important to submit this CL right this second, then ok - but longer
term
> I'd really like to see a RAAI solution. For example, a file like
> "extensions/renderer/scoped_web_frame":
> 
> class ScopedWebFrame {
>  public:
>   ScopedWebFrame() : view_(nullptr), frame_(nullptr) {
>     view_ = WebView::Create(nullptr);
>     frame_ = WebLocalFrame::create(...);
>     view_->setMainFrame(frame);
>   }
> 
>   ~ScopedWebFrame() {
>     view_->close();
>     frame_->close();
>     WebHeap::collectAllGarbageForTesting();
>   }
> 
>   WebLocalFrame* frame() { return frame_; }
> 
>  private:
>   WebView* view_;
>   WebLocalFrame* frame_;
> };
> 
> then other tests can use that as well, *and* we have a canonical way of
creating
> test frames.

Done. Would you take another look?

[not at google - send to devlin, 2015-06-25 15:20:16]

Thanks. Only important thing would be to put them in separate .h and .cc files.

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
File extensions/renderer/scoped_web_frame.h (right):

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:15: class ScopedWebFrame {
This should be split into a .h and a .cc file. The code I wrote was optimised
for brevity.

Also when you do please add an overall class comment.

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:33: blink::WebView* view_;
Is there a comment here you could make about the lifetime? I never understood
why you're not supposed to delete these, since you created them.

If you don't know that's fine. Neither do I. In that case, I'd just vaguely
mention "These manage their own lifetime" so that anybody reading the code
doesn't put them in a scoped_ptr and cause a bug. Which I did once :-)

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:34: blink::WebLocalFrame* frame_;
Also DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context_set_unittest.cc (right):

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context_set_unittest.cc:25: ScopedWebFrame web_frame;
Why do you need to wrap this in {}? Is it because must be destroyed before
loop.RunUntilIdle()?

IIRC that RunUntilIdle is needed because ScriptContextSet::Remove is
asynchronous, it has nothing to do with WebFrame, so I would hope you don't need
to wrap this in a block.

[haraken, 2015-06-26 00:35:34]

Thanks for review!

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
File extensions/renderer/scoped_web_frame.h (right):

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:15: class ScopedWebFrame {
On 2015/06/25 15:20:16, kalman wrote:
> This should be split into a .h and a .cc file. The code I wrote was optimised
> for brevity.
> 
> Also when you do please add an overall class comment.

Done.

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:33: blink::WebView* view_;
On 2015/06/25 15:20:16, kalman wrote:
> Is there a comment here you could make about the lifetime? I never understood
> why you're not supposed to delete these, since you created them.
> 
> If you don't know that's fine. Neither do I. In that case, I'd just vaguely
> mention "These manage their own lifetime" so that anybody reading the code
> doesn't put them in a scoped_ptr and cause a bug. Which I did once :-)

Added a comment. It's not nice that the content layer sometimes holds a raw
pointer into Blink. It's causing a lot of use-after-frees (this is one of the
biggest causes of use-after-frees detected by Oilpan)...

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/sco...
extensions/renderer/scoped_web_frame.h:34: blink::WebLocalFrame* frame_;
On 2015/06/25 15:20:16, kalman wrote:
> Also DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context_set_unittest.cc (right):

https://codereview.chromium.org/1210513002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context_set_unittest.cc:25: ScopedWebFrame web_frame;
On 2015/06/25 15:20:16, kalman wrote:
> Why do you need to wrap this in {}? Is it because must be destroyed before
> loop.RunUntilIdle()?
> 
> IIRC that RunUntilIdle is needed because ScriptContextSet::Remove is
> asynchronous, it has nothing to do with WebFrame, so I would hope you don't
need
> to wrap this in a block.

Done.

[haraken, 2015-06-26 00:35:38]

Thanks for review!

[not at google - send to devlin, 2015-06-26 01:22:24]

Looks like you didn't add the .cc file, but, lgtm in anticipation.

[haraken, 2015-06-26 01:38:35]

On 2015/06/26 01:22:24, kalman wrote:
> Looks like you didn't add the .cc file, but, lgtm in anticipation.

Done :)

[haraken, 2015-06-26 01:38:58]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-26 01:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1210513002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-26 01:39:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210513002/80001

[commit-bot: I haz the power, 2015-06-26 02:03:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-26 02:04:04]

Patchset 5 (id:??) landed as
https://crrev.com/1156707553435bdc1a7830186e38f0e5dc01f8f4
Cr-Commit-Position: refs/heads/master@{#336321}