Added ScopedNSAutoreleasePool to RenderViewTest.

Added ScopedNSAutoreleasePool to RenderViewTest.

If Blink checks font metrics during some browser_test (e.g. PrintLayoutTest), Blink might allocates CFString by using Mac allocator, and the CFString might have a reference to StringImpl allocated by FastMalloc.

So we should invoke Mac decallocator before destroying Blink instance. If we don't, browser_tests will crash.

BUG=305885, 336756
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249043

[tasak, 2014-01-31 11:49:52]

Would you review this CL?

[Nico, 2014-02-03 20:12:57]

Can you put this in RenderViewTest instead?

[tasak, 2014-02-04 05:01:37]

On 2014/02/03 20:12:57, Nico wrote:
> Can you put this in RenderViewTest instead?

Sure.
Done.

[Nico, 2014-02-04 16:12:51]

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.cc:221: autorelease_pool_.reset(NULL);
Is this necessary? The destructor will run this automatically.

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.h:143:
scoped_ptr<base::mac::ScopedNSAutoreleasePool> autorelease_pool_;
Does this have to be a scoped_ptr? A regular member variable might be good
enough.

[tasak, 2014-02-05 04:23:02]

Thank you for reviewing.

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.cc:221: autorelease_pool_.reset(NULL);
On 2014/02/04 16:12:51, Nico wrote:
> Is this necessary? The destructor will run this automatically.

Yes. This should be done before blink::shutdown().

If this line is removed, browser_tests will crash (I have crash log).

If we move this to destructor, we must also move blink::shutdown() to
destructor.

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.h:143:
scoped_ptr<base::mac::ScopedNSAutoreleasePool> autorelease_pool_;
On 2014/02/04 16:12:51, Nico wrote:
> Does this have to be a scoped_ptr? A regular member variable might be good
> enough.

I would like to make ScopedNSAutoreleasePool's lifetime the same as test's
lifetime, i.e. from SetUp to TearDown.

Looking at ScopedNSAutoreleasePool's behavior, if we use the variable as a
regular member, it will start working at  RenderViewTest's constructor time.

If we create some MacOSX thing after creating RenderViewTest before invoking its
SetUp, we might have some trouble?

[Nico, 2014-02-05 04:58:22]

lgtm

Thanks for explaining, and for your patience!

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.cc:221: autorelease_pool_.reset(NULL);
On 2014/02/05 04:23:02, tasak wrote:
> On 2014/02/04 16:12:51, Nico wrote:
> > Is this necessary? The destructor will run this automatically.
> 
> Yes. This should be done before blink::shutdown().
> 
> If this line is removed, browser_tests will crash (I have crash log).
> 
> If we move this to destructor, we must also move blink::shutdown() to
> destructor.

Cool. Say "// Needs to run before blink::shutdown()." above the reset() line.

[tasak, 2014-02-05 05:16:02]

Thank you for reviewing.

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/137023014/diff/30001/content/public/test/rend...
content/public/test/render_view_test.cc:221: autorelease_pool_.reset(NULL);
On 2014/02/05 04:58:22, Nico wrote:
> On 2014/02/05 04:23:02, tasak wrote:
> > On 2014/02/04 16:12:51, Nico wrote:
> > > Is this necessary? The destructor will run this automatically.
> > 
> > Yes. This should be done before blink::shutdown().
> > 
> > If this line is removed, browser_tests will crash (I have crash log).
> > 
> > If we move this to destructor, we must also move blink::shutdown() to
> > destructor.
> 
> Cool. Say "// Needs to run before blink::shutdown()." above the reset() line.

Sure. Done.

[tasak, 2014-02-05 05:16:13]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-05 05:20:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/137023014/190001

[commit-bot: I haz the power, 2014-02-05 06:05:05]

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

[commit-bot: I haz the power, 2014-02-05 06:05:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nico, 2014-02-05 06:11:20]

+avi for content/OWNERS

[tasak, 2014-02-05 06:12:15]

On 2014/02/05 06:11:20, Nico wrote:
> +avi for content/OWNERS

Thank you.

[Avi (use Gerrit), 2014-02-05 15:59:38]

On 2014/02/05 06:12:15, tasak wrote:
> On 2014/02/05 06:11:20, Nico wrote:
> > +avi for content/OWNERS
> 
> Thank you.

LGTM

[Nico, 2014-02-05 16:00:37]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-02-05 16:02:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/137023014/190001

[commit-bot: I haz the power, 2014-02-05 17:50:42]

Change committed as 249043