Unload the ScopedGaiaAuthExtension asynchronously.

Unload the ScopedGaiaAuthExtension asynchronously.

If we don't do this, then we have a race condition between unloading
this extension, and trying to use it because we have manually
navigated to it.

BUG=460431
Committed: https://crrev.com/1805c4c5c4b5292cca51a54f379cde498db7238b
Cr-Commit-Position: refs/heads/master@{#318140}

Committed: https://crrev.com/c37009c34bc9c7921cabf5f9cfee96430511be97
Cr-Commit-Position: refs/heads/master@{#320937}

[noms (inactive), 2015-02-25 20:47:24]

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

[noms (inactive), 2015-02-25 20:47:24]

Spoilers: it's the CL we just talked about. Please take a look :)

[xiyuan, 2015-02-25 21:17:30]

Would we crash on browser shutdown with a "chrome://chrome-signin/?source=0" tab
open? If so, we might need to move the PostTask code into
GaiaAuthExtensionLoader::UnloadIfNeeded.

Otherwise, lgtm.

https://codereview.chromium.org/961443003/diff/1/chrome/browser/extensions/si...
File chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc (right):

https://codereview.chromium.org/961443003/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc:30:
content::BrowserThread::PostTask(
nit: Slightly prefer to use base::ThreadTaskRunnerHandle::Get()->PostTask
instead of content::BrowserThread::PostTask because that would make the code
depends on base rather than content.

[noms, 2015-02-25 22:19:26]

noms@google.com changed reviewers:
+ noms@google.com

[noms, 2015-02-25 22:19:27]

https://codereview.chromium.org/961443003/diff/1/chrome/browser/extensions/si...
File chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc (right):

https://codereview.chromium.org/961443003/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc:30:
content::BrowserThread::PostTask(
On 2015/02/25 21:17:30, xiyuan wrote:
> nit: Slightly prefer to use base::ThreadTaskRunnerHandle::Get()->PostTask
> instead of content::BrowserThread::PostTask because that would make the code
> depends on base rather than content.

Done.

[xiyuan, 2015-02-25 22:21:46]

slgtm

Have you get a chance to verify the shutdown case (w/ browser sign-in tab open)?

[noms (inactive), 2015-02-25 22:22:48]

On 2015/02/25 22:21:46, xiyuan wrote:
> slgtm
> 
> Have you get a chance to verify the shutdown case (w/ browser sign-in tab
open)?

Oh yeah, thought I had replied, sorry: it doesn't crash with the signin tab
open. whether this stays true in the wild...

[xiyuan, 2015-02-25 22:24:52]

Awesome. Thank you for trying it out.

[noms, 2015-02-25 22:37:14]

The CQ bit was checked by noms@google.com

[noms, 2015-02-25 22:37:15]

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/961443003/#ps20001 (title:
"xiyuan nit")

[commit-bot: I haz the power, 2015-02-25 22:38:18]

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

[commit-bot: I haz the power, 2015-02-25 23:24:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-25 23:27:14]

Patchset 2 (id:??) landed as
https://crrev.com/1805c4c5c4b5292cca51a54f379cde498db7238b
Cr-Commit-Position: refs/heads/master@{#318140}

[Sam McNally, 2015-02-26 02:36:41]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/944923007/ by sammc@chromium.org.

The reason for reverting is: Broke BookmarkSyncPromoControllerTest.SignInLink on
Mac ASan 64:
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests....

[noms (inactive), 2015-03-04 23:39:49]

noms@chromium.org changed reviewers:
+ asvitkine@chromium.org

[noms (inactive), 2015-03-04 23:39:49]

+ alexei for an owner stamp on the cocoa test :)

[Alexei Svitkine (slow), 2015-03-05 16:11:38]

https://codereview.chromium.org/961443003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc (right):

https://codereview.chromium.org/961443003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc:35:
base::Unretained(loader)));
Why is it safe to use base::Unretained() here?

What guarantees that the loader won't go away before the task runs?

If there's no good answer to that question, then probably it would be better to
make a method on loader that says loader->UnloadIfNeededAsync() that does a post
task internally with a weak pointer.

[noms (inactive), 2015-03-16 15:57:42]

https://codereview.chromium.org/961443003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc (right):

https://codereview.chromium.org/961443003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/signin/scoped_gaia_auth_extension.cc:35:
base::Unretained(loader)));
On 2015/03/05 16:11:38, Alexei Svitkine (slow) wrote:
> Why is it safe to use base::Unretained() here?
> 
> What guarantees that the loader won't go away before the task runs?
> 
> If there's no good answer to that question, then probably it would be better
to
> make a method on loader that says loader->UnloadIfNeededAsync() that does a
post
> task internally with a weak pointer.

Done.

[Alexei Svitkine (slow), 2015-03-16 16:52:58]

lgtm

[noms (inactive), 2015-03-16 16:58:18]

The CQ bit was checked by noms@chromium.org

[noms (inactive), 2015-03-16 16:58:18]

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/961443003/#ps60001 (title:
"add loader weak_ptr_factory")

[noms (inactive), 2015-03-16 16:58:35]

The CQ bit was unchecked by noms@chromium.org

[commit-bot: I haz the power, 2015-03-16 16:58:46]

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

[noms (inactive), 2015-03-17 16:46:33]

The CQ bit was checked by noms@chromium.org

[noms (inactive), 2015-03-17 16:46:34]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/961443003/#ps80001 (title:
"fix whitespace")

[commit-bot: I haz the power, 2015-03-17 16:48:56]

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

[commit-bot: I haz the power, 2015-03-17 17:47:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-17 17:47:51]

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