ExtensionsTest: Move initialization to SetUp and avoid potential UAF.

ExtensionsTest: Move initialization to SetUp and avoid potential UAF.

ExtensionsTest does all its initialization in its constructor but destroys its
initialized instances in TearDown(). This CL moves all the initialization logic
to its SetUp() method to make the initialization consistent with destruction.
The ExtensionsTest subclasses which rely on the current initialization sequence
are also modified.

Also, currently the extensions_browser_client_ instance is reset in TearDown()
but not unset as the singleton until the destructor. This can cause use after
free errors. This is also fixed by resetting the singleton instances in
TearDown() itself.

BUG=708256
Review-Url: https://codereview.chromium.org/2802433004
Cr-Commit-Position: refs/heads/master@{#463854}
Committed: https://chromium.googlesource.com/chromium/src/+/fb19fb99a92bd5335dd24dde693e64f8ced55cbf

[karandeepb, 2017-04-04 23:51:20]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 23:51:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 23:56:26]

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

[commit-bot: I haz the power, 2017-04-04 23:56:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[karandeepb, 2017-04-06 03:13:17]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 03:13:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2017-04-06 03:29:23]

karandeepb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[karandeepb, 2017-04-06 03:29:24]

PTAL Devlin

https://codereview.chromium.org/2802433004/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc
(right):

https://codereview.chromium.org/2802433004/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:46:
mock_adapter_(new testing::StrictMock<device::MockBluetoothAdapter>()) {
Seems this is leaked. Intentional?

[commit-bot: I haz the power, 2017-04-06 03:49:33]

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

[commit-bot: I haz the power, 2017-04-06 03:49:34]

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

[karandeepb, 2017-04-06 04:43:50]

On 2017/04/06 03:29:24, karandeepb wrote:
> PTAL Devlin
> 
>
https://codereview.chromium.org/2802433004/diff/1/extensions/browser/api/blue...
> File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2802433004/diff/1/extensions/browser/api/blue...
> extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:46:
> mock_adapter_(new testing::StrictMock<device::MockBluetoothAdapter>()) {
> Seems this is leaked. Intentional?

Devlin: Please hold off this review. There seems to be a single test failure.

[Devlin, 2017-04-06 14:12:20]

On 2017/04/06 04:43:50, karandeepb wrote:
> Devlin: Please hold off this review. There seems to be a single test failure.

Ack; ping again when ready. :)

[karandeepb, 2017-04-07 23:33:19]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 23:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 23:37:20]

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

[commit-bot: I haz the power, 2017-04-07 23:37:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[karandeepb, 2017-04-08 00:03:12]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-08 00:03:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-08 00:51:31]

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

[commit-bot: I haz the power, 2017-04-08 00:51:32]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-04-10 18:53:48]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 18:54:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 19:37:57]

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

[commit-bot: I haz the power, 2017-04-10 19:37:58]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-04-10 22:26:23]

PTAL Devlin.

https://codereview.chromium.org/2802433004/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2802433004/diff/80001/extensions/browser/exte...
extensions/browser/extensions_test.cc:38:
content::SetBrowserClientForTesting(nullptr);
I also tried to move:
  content::SetBrowserClientForTesting(nullptr);
  content::SetUtilityClientForTesting(nullptr);
to the TearDown() method but it led to failure of a single test:
IdentityApiTest.GetAuthToken. IdentityApiTest inherits from ApiUnitTest which
has a TestBrowserThreadBundle instance. 

The destructor of TestBrowserThreadBundle relies on an active
ContentBrowserClient, which causes the test failure for PS2. 

But resetting the TestBrowserThreadBundle in ApiUnitTest::TearDown doesn't
simply solve the issue. After destructing TestBrowserThreadBundle, the UI thread
is stopped and the current thread is not the UI thread (not sure if this is a
bug). The destructor of a BrowserContext relies on the current thread being UI. 

So for the current behavior, the destruction order needs to be:
-> Browser context, TestBrowserThreadBundle, ContentBrowserClient.

[Devlin, 2017-04-11 22:00:10]

lgtm

[karandeepb, 2017-04-11 22:35:39]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-04-11 22:36:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 00:23:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491950139476570,
"parent_rev": "962f6109508a91c8110ac855d018d334b274913e", "commit_rev":
"fb19fb99a92bd5335dd24dde693e64f8ced55cbf"}

[commit-bot: I haz the power, 2017-04-12 00:24:53]

Description was changed from

==========
ExtensionsTest: Move initialization to SetUp and avoid potential UAF.

ExtensionsTest does all its initialization in its constructor but destroys its
initialized instances in TearDown(). This CL moves all the initialization logic
to its SetUp() method to make the initialization consistent with destruction.
The ExtensionsTest subclasses which rely on the current initialization sequence
are also modified.

Also, currently the extensions_browser_client_ instance is reset in TearDown()
but not unset as the singleton until the destructor. This can cause use after
free errors. This is also fixed by resetting the singleton instances in
TearDown() itself.

BUG=708256
==========

to

==========
ExtensionsTest: Move initialization to SetUp and avoid potential UAF.

ExtensionsTest does all its initialization in its constructor but destroys its
initialized instances in TearDown(). This CL moves all the initialization logic
to its SetUp() method to make the initialization consistent with destruction.
The ExtensionsTest subclasses which rely on the current initialization sequence
are also modified.

Also, currently the extensions_browser_client_ instance is reset in TearDown()
but not unset as the singleton until the destructor. This can cause use after
free errors. This is also fixed by resetting the singleton instances in
TearDown() itself.

BUG=708256

Review-Url: https://codereview.chromium.org/2802433004
Cr-Commit-Position: refs/heads/master@{#463854}
Committed:
https://chromium.googlesource.com/chromium/src/+/fb19fb99a92bd5335dd24dde693e...
==========

[commit-bot: I haz the power, 2017-04-12 00:24:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fb19fb99a92bd5335dd24dde693e...