Add thread checks to NaClBrowser, and make it leaky

Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.

3. Changes the signature of SetDelegate() to express that the caller no
   longer owns the supplied NaClBrowserDelegate.

4. Leaks the NaClBrowser and NaClBrowserDelegate, rather than cleaning
   them up on exit, since at present they have no cleanup to do.

Review-Url: https://codereview.chromium.org/2630443003
Cr-Commit-Position: refs/heads/master@{#452697}
Committed: https://chromium.googlesource.com/chromium/src/+/72c5d6e04f39f920ea41eb20d56c593fbf83a70c

[Wez, 2017-01-12 02:30:59]

Description was changed from

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Makes NaClBrowser leaky.

3. Adds thread checks on the NaClBrowser API calls.
==========

to

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.
==========

[Wez, 2017-01-12 02:41:05]

wez@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Wez, 2017-01-12 02:41:06]

Brad, could you PTAL?

NaClBrowser / NaClBrowserDelegate have some threading issues that I'm trying to
un-pick so I can harden WeakPtr:

1. NaClBrowser posts work to a File thread and received results on the IO
thread, using WeakPtr to decouple things. However, it's actually deleted on the
UI thread (via AtExit), so the WeakPtr doesn't work as intended.
   I've made it leaky in this patch, but we could probably tear it down via a
MessageLoop::DestructionObserver on the IO thread instead, and keep the WeakPtr
- WDYT?

2. NaClBrowserDelegate was:
- Created and supplied via the UI thread.
- Called by the NaClBrowserDelegate from the IO thread.
- Called by other bits of NaCl on various worker pools.
- Destroyed by NaClBrowser on the UI thread.
   Again, I've made it leaky in this patch, but it could instead by stored in a
LazyInstance<unique_ptr<>> if it's important that it be torn down; it seems like
an embedder-provided interface rather than an object, so having the embedder own
it, and tear it down, would seem the cleanest approach - WDYT?

P.S. Should we update components/nacl/OWNERS now that various ppl have moved on
to other projects? :P

[Wez, 2017-01-12 02:41:51]

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

[commit-bot: I haz the power, 2017-01-12 02:42:15]

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-01-12 02:51:25]

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

[commit-bot: I haz the power, 2017-01-12 02:51:26]

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

[Wez, 2017-01-13 19:25:58]

Description was changed from

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.
==========

to

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.

3. Changes the signature of SetDelegate() to express that the caller no
   longer owns the supplied NaClBrowserDelegate.

4. Leaks the NaClBrowser and NaClBrowserDelegate, rather than cleaning
   them up on exit, since at present they have no cleanup to do.
==========

[Wez, 2017-01-13 19:26:06]

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

[commit-bot: I haz the power, 2017-01-13 19:26:45]

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

[Wez, 2017-01-13 19:56:02]

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

[commit-bot: I haz the power, 2017-01-13 19:56:53]

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-01-13 20:38:17]

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

[commit-bot: I haz the power, 2017-01-13 20:38:18]

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

[Wez, 2017-01-14 01:53:11]

https://codereview.chromium.org/2630443003/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2630443003/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1655:
browser_process_->profile_manager()));
bradnelson: Is it possible to move this to be pre-thread-initialization? Is
there anything that the NaClBrowserDelegateImpl needs BrowserThreads for?

https://codereview.chromium.org/2630443003/diff/40001/chrome/browser/nacl_hos...
File chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc (right):

https://codereview.chromium.org/2630443003/diff/40001/chrome/browser/nacl_hos...
chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc:57:
nacl::NaClBrowser::GetInstance()->SetGdbDebugStubPortListenerForTest(
bradnelson: These GetInstance() calls barf because we're on the main thread. We
could:

1. Update the test to PostTask them to the IO thread.
2. Make the GDB debug stub port listener APIs static, since they're just for
test, so their use on the wrong thread here is not really an issue.
3. Relax the thread check in GetInstance(). I'd prefer not to do that since new
regressions will just creep back in.

https://codereview.chromium.org/2630443003/diff/40001/components/nacl/browser...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/2630443003/diff/40001/components/nacl/browser...
components/nacl/browser/nacl_browser.cc:170:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
bradnelson: Turns out that this breaks several tests, which call SetDelegate()
before initializing any BrowserThreads. Ideally I'd prefer that we require that
this be called before BrowserThreads exist even in Chrome itself, so that there
is no risk of racing with other threads - see my comment in Chrome MainParts.

[Wez, 2017-01-18 00:42:24]

Pingy. :)

[Wez, 2017-01-26 22:26:04]

Ping - WDYT re the threading issues here?

[Wez, 2017-02-17 03:45:49]

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

[commit-bot: I haz the power, 2017-02-17 03:47:05]

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-02-17 04:51:41]

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

[commit-bot: I haz the power, 2017-02-17 04:51:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wez, 2017-02-17 06:51:02]

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

[commit-bot: I haz the power, 2017-02-17 06:51:37]

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-02-17 07:32:46]

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

[commit-bot: I haz the power, 2017-02-17 07:32:47]

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

[Wez, 2017-02-17 07:56:05]

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

[commit-bot: I haz the power, 2017-02-17 07:56:26]

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-02-17 08:52:41]

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

[commit-bot: I haz the power, 2017-02-17 08:52:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Wez, 2017-02-18 01:19:19]

Bard, PTAL - had to make ugly changes to allow tests to be lazy about threading,
I'm afraid.

[bradnelson, 2017-02-23 03:07:43]

lgtm

https://codereview.chromium.org/2630443003/diff/100001/components/nacl/browse...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/2630443003/diff/100001/components/nacl/browse...
components/nacl/browser/nacl_browser.cc:201: NOTREACHED();
Good idea

[Wez, 2017-02-23 03:15:41]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-02-23 03:16:16]

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-02-23 03:25:45]

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

[commit-bot: I haz the power, 2017-02-23 03:25:46]

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...)

[Wez, 2017-02-23 03:32:01]

wez@chromium.org changed reviewers:
+ derat@chromium.org, sky@chromium.org

[Wez, 2017-02-23 03:32:02]

derat: Plz OWNERS shell_browser_main_parts.cc
sky: Plz OWNERS chrome_browser_main.cc

Thanks :)

[Daniel Erat, 2017-02-23 04:21:45]

lgtm for extensions/shell/

[sky, 2017-02-23 19:58:02]

LGTM

[Wez, 2017-02-24 00:29:33]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:30:03]

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-02-24 00:53:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487896173484190,
"parent_rev": "33237fadcbda24bd31d2a166ba9a79c994671dda", "commit_rev":
"72c5d6e04f39f920ea41eb20d56c593fbf83a70c"}

[commit-bot: I haz the power, 2017-02-24 00:54:22]

Description was changed from

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.

3. Changes the signature of SetDelegate() to express that the caller no
   longer owns the supplied NaClBrowserDelegate.

4. Leaks the NaClBrowser and NaClBrowserDelegate, rather than cleaning
   them up on exit, since at present they have no cleanup to do.
==========

to

==========
Add thread checks to NaClBrowser, and make it leaky.

This makes the following fixes to NaClBrowser:

1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned
   when NaClBrowser is torn-down; this never worked as intended, since
   the WeakPtrs were used on the I/O thread while NaClBrowser was torn-
   down via the Singleton::OnExit handler, on the UI thread.

2. Adds thread checks to NaClBrowser to ensure it is only called from
   the browser's I/O thread.

3. Changes the signature of SetDelegate() to express that the caller no
   longer owns the supplied NaClBrowserDelegate.

4. Leaks the NaClBrowser and NaClBrowserDelegate, rather than cleaning
   them up on exit, since at present they have no cleanup to do.

Review-Url: https://codereview.chromium.org/2630443003
Cr-Commit-Position: refs/heads/master@{#452697}
Committed:
https://chromium.googlesource.com/chromium/src/+/72c5d6e04f39f920ea41eb20d56c...
==========

[commit-bot: I haz the power, 2017-02-24 00:54:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/72c5d6e04f39f920ea41eb20d56c...

[Wez, 2017-03-01 17:38:05]

Brad: I'm concerned that this may actually create artificial dependencies across
NaClBrowser tests, since the NaClBrowser is no longer torn-down.

I'm wondering whether we need a clean shutdown mechanism for NaClBrowser, for
tests..?