Always destroy CrxInstaller on the UI Thread

Always destroy CrxInstaller on the UI Thread

CrxInstaller currently just guarantees that its ExtensionInstallPrompt
|client_| data member is destroyed on the UI Thread. But this is not
enough -- CrxInstaller is the InstallPrompt's delegate via a weak
reference, so it can't be destroyed first.

This changes CrxInstaller from a RefCountedThreadSafe to a
RefCountedDeleteOnMessageLoop to ensure the whole thing is destroyed on
the UI thread when all references are gone.

Note that CrxInstaller also owns a ChromeRequirementsChecker which is a
WeakReferenceOwner. WeakReferenceOwners must be destroyed on the thread
that created them, so CrxInstaller being destroyed on the FILE thread
also hits a DCHECK at weak_ptr.cc:20.

Since SandboxedUnpackerClient is the base class that "is" the
RefCountedThreadSafe other classes derived from SandboxedUnpackerClient
are affected. To keep things simple and consistent, they all become
RefCountedDeleteOnMessageLoop.

BUG=410415
TEST=At the webstore: {show an install dialog, then cancel it}. Repeat
many times. Chrome should not crash. Previously a race condition could
be triggered causing Chrome to crash (maybe a 1-in-20 chance).

Committed: https://crrev.com/1e394abd91dbfa362d139fbc7b10fa5d5d2e0631
Cr-Commit-Position: refs/heads/master@{#325339}

[tapted, 2015-04-10 05:32:33]

tapted@chromium.org changed reviewers:
+ benwells@chromium.org

[tapted, 2015-04-13 04:49:48]

Patchset #4 (id:60001) has been deleted

[tapted, 2015-04-13 06:28:12]

tapted@chromium.org changed reviewers:
+ xiyuan@chromium.org

[tapted, 2015-04-13 06:28:52]

PTAL: benwells for c/b/extensions, xiyuan for c/b/chromeos

[xiyuan, 2015-04-13 16:35:09]

lgtm

[benwells, 2015-04-14 22:14:19]

https://codereview.chromium.org/1080453002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/1080453002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:169: // The scoped_ptr |client_| data
member must be destroyed on the UI thread.
Nit: the client_ member was destroyed on the ui thread before. maybe add a
comment explaining why that alone is not enough.

https://codereview.chromium.org/1080453002/diff/80001/extensions/browser/sand...
File extensions/browser/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/1080453002/diff/80001/extensions/browser/sand...
extensions/browser/sandboxed_unpacker.cc:217: :
RefCountedDeleteOnMessageLoop<SandboxedUnpackerClient>(loop) {
Can you put BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)
straight in here instead of plumbing it up from all subclasses?

[tapted, 2015-04-15 01:20:55]

Patchset #5 (id:100001) has been deleted

[tapted, 2015-04-15 03:07:44]

https://codereview.chromium.org/1080453002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/1080453002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:169: // The scoped_ptr |client_| data
member must be destroyed on the UI thread.
On 2015/04/14 22:14:19, benwells wrote:
> Nit: the client_ member was destroyed on the ui thread before. maybe add a
> comment explaining why that alone is not enough.

Done.

https://codereview.chromium.org/1080453002/diff/80001/extensions/browser/sand...
File extensions/browser/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/1080453002/diff/80001/extensions/browser/sand...
extensions/browser/sandboxed_unpacker.cc:217: :
RefCountedDeleteOnMessageLoop<SandboxedUnpackerClient>(loop) {
On 2015/04/14 22:14:19, benwells wrote:
> Can you put BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)
> straight in here instead of plumbing it up from all subclasses?

Done. Turns out //extensions/browser is fine wrt DEPS. But at least this forced
me to double-check all the derived classes to ensure they're OK with the change.

Added a DCHECK_CURRENTLY_ON(BrowserThread::UI); to the constructor here, and
updated the header comment.

[benwells, 2015-04-15 21:56:47]

lgtm

[tapted, 2015-04-15 23:14:22]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-04-15 23:14:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1080453002/#ps120001
(title: "Less plumbing. More comments")

[commit-bot: I haz the power, 2015-04-15 23:14:54]

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

[commit-bot: I haz the power, 2015-04-15 23:22:31]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-04-15 23:23:36]

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