Remove bare extension pointer from platform app launcher code.

Remove bare extension pointer from platform app launcher code.

The bare pointer could cause crashes if the extension is uninstalled (thus being deleted) while the launch logic is progressing on the FILE thread; storing an extension ID prevents this.

BUG=372270
Committed: https://crrev.com/8af39484cdb143fd4d811cf404a41f18c5cb2abe
Cr-Commit-Position: refs/heads/master@{#304993}

[benwells, 2014-11-20 05:50:17]

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

[tapted, 2014-11-20 06:15:29]

just one question really.

also might be worth clarifying in the description why a crash can occur (i.e. ~
"The bare pointer could cause crashes when extensions were uninstalled while
waiting for tasks posted to the FILE thread to complete").

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode118
apps/launcher.cc:118: DCHECK_CURRENTLY_ON(BrowserThread::UI);
nit: maybe have this DCHECK first?

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode197
apps/launcher.cc:197: DCHECK_CURRENTLY_ON(BrowserThread::UI);
nit: here too

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode303
apps/launcher.cc:303: extension_id, extensions::ExtensionRegistry::EVERYTHING);
maybe just extensions::ExtensionRegistry::ENABLED?  (i.e. if the extension is
disabled, or crashes while posting tasks, is it worth bailing out?)

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode311
apps/launcher.cc:311: std::string extension_id;
nit: declare const?

[benwells, 2014-11-20 07:07:18]

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode118
apps/launcher.cc:118: DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/11/20 06:15:29, tapted wrote:
> nit: maybe have this DCHECK first?

Done.

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode197
apps/launcher.cc:197: DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/11/20 06:15:29, tapted wrote:
> nit: here too

Done.

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode303
apps/launcher.cc:303: extension_id, extensions::ExtensionRegistry::EVERYTHING);
On 2014/11/20 06:15:29, tapted wrote:
> maybe just extensions::ExtensionRegistry::ENABLED?  (i.e. if the extension is
> disabled, or crashes while posting tasks, is it worth bailing out?)

I thought about that but didn't want to apply any smart logic about extension
state in this area of the code. I just want it not to crash.

https://codereview.chromium.org/736233002/diff/20001/apps/launcher.cc#newcode311
apps/launcher.cc:311: std::string extension_id;
On 2014/11/20 06:15:29, tapted wrote:
> nit: declare const?

Done.

[tapted, 2014-11-20 07:13:23]

lgtm

[benwells, 2014-11-20 07:47:50]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-11-20 07:48:32]

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

[commit-bot: I haz the power, 2014-11-20 08:32:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-20 08:33:18]

Patchset 3 (id:??) landed as
https://crrev.com/8af39484cdb143fd4d811cf404a41f18c5cb2abe
Cr-Commit-Position: refs/heads/master@{#304993}