Request the Identity API's Uber Token without a Channel ID

Request the Identity API's Uber Token without a Channel ID

BUG=621542
Committed: https://crrev.com/b07bc57f8b5e3407bc6a60901bc2da19046ac08d
Cr-Commit-Position: refs/heads/master@{#405596}

[anthonyvd, 2016-07-13 18:36:07]

anthonyvd@chromium.org changed reviewers:
+ courage@chromium.org, nharper@chromium.org

[anthonyvd, 2016-07-13 18:36:08]

Hi guys, can you please take a look at this change to make the Identity API's
Uber Token request without a Channel ID?

Thanks!

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:293: protected:
This diff is pretty bad. This is making AppRequestContext public.

[nharper, 2016-07-13 20:04:48]

I assume this is a temporary measure to fix the bug quickly. The uber token
should eventually be channel bound again.

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:293: protected:
On 2016/07/13 18:36:08, anthonyvd wrote:
> This diff is pretty bad. This is making AppRequestContext public.

The reason for using the AppRequestContext was to have something to hold
ownership of the URLRequestContext's pointers. I think you could use an
URLRequestContext directly and have the GaiaWebAuthFlow hold ownership (or use
an URLRequestContextStorage to hold ownership of the new objects).

[anthonyvd, 2016-07-13 20:33:42]

On 2016/07/13 at 20:04:48, nharper wrote:
> I assume this is a temporary measure to fix the bug quickly. The uber token
should eventually be channel bound again.

Yes, although I'm still not sure how to get the app's request context to make
that request in.

> 
>
https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_io_data.h (right):
> 
>
https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_io_data.h:293: protected:
> On 2016/07/13 18:36:08, anthonyvd wrote:
> > This diff is pretty bad. This is making AppRequestContext public.
> 
> The reason for using the AppRequestContext was to have something to hold
ownership of the URLRequestContext's pointers. I think you could use an
URLRequestContext directly and have the GaiaWebAuthFlow hold ownership (or use
an URLRequestContextStorage to hold ownership of the new objects).

[anthonyvd, 2016-07-13 20:33:47]

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/2145103003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:293: protected:
On 2016/07/13 at 20:04:47, nharper wrote:
> On 2016/07/13 18:36:08, anthonyvd wrote:
> > This diff is pretty bad. This is making AppRequestContext public.
> 
> The reason for using the AppRequestContext was to have something to hold
ownership of the URLRequestContext's pointers. I think you could use an
URLRequestContext directly and have the GaiaWebAuthFlow hold ownership (or use
an URLRequestContextStorage to hold ownership of the new objects).

I see. I initially tried to use it because it's not possible to set
HttpNetworkSession on net::URLRequestContext. Turns out it's
HttpTransactionFactory that ties all of this together so using AppRequestContext
was irrelevant.

Done.

[Michael Courage, 2016-07-13 20:43:01]

There are a two object lifetime issues I'm concerned about in this change. I
have now forgotten the full details, but the GaiaWebAuthFlow won't always
complete before its deletion. In particular if the browser shuts down, an
app/extension API call can be destroyed without necessarily completing. You can
trace through IdentityAPI::Shutdown to see how this would go.

Imagine that that shutdown path executes while:
1. StartOnIOThread is running, using that base::Unretained(this) pointer.
2. The Ubertoken fetch is running.
3. While CleanupRequestContextOnIOThread is running.

I think 1&3 could cause the I/O thread to access a deallocated GaiaWebAuthFlow.

I think 2 could cause the ubertoken_request_context_ to be deleted on the UI
thread instead of the IO thread, because the ubertoken callbacks will never run,
and so  ::~GaiaWebAuthFlow will end up deleting the request context.

[anthonyvd, 2016-07-13 20:48:10]

On 2016/07/13 at 20:43:01, courage wrote:
> There are a two object lifetime issues I'm concerned about in this change. I
have now forgotten the full details, but the GaiaWebAuthFlow won't always
complete before its deletion. In particular if the browser shuts down, an
app/extension API call can be destroyed without necessarily completing. You can
trace through IdentityAPI::Shutdown to see how this would go.

Thanks for the feedback, I wasn't sure about that either.

> 
> Imagine that that shutdown path executes while:
> 1. StartOnIOThread is running, using that base::Unretained(this) pointer.
> 2. The Ubertoken fetch is running.
> 3. While CleanupRequestContextOnIOThread is running.
> 
> I think 1&3 could cause the I/O thread to access a deallocated
GaiaWebAuthFlow.
> 
> I think 2 could cause the ubertoken_request_context_ to be deleted on the UI
thread instead of the IO thread, because the ubertoken callbacks will never run,
and so  ::~GaiaWebAuthFlow will end up deleting the request context.

What do you think about making GaiaWebFlow a base::RefCountedThreadSafe (and not
calling base::Unretained(this)) to take care of 1&3 and calling
CleanupOnIOThread from ~GaiaWebAuthFlow to address 2? Is there anything
obviously wrong with that approach?

[Michael Courage, 2016-07-13 21:57:53]

> What do you think about making GaiaWebFlow a base::RefCountedThreadSafe (and
not
> calling base::Unretained(this)) to take care of 1&3

Would be nice if you could avoid refcounting, but it would be ok.

> and calling
> CleanupOnIOThread from ~GaiaWebAuthFlow to address 2? Is there anything
> obviously wrong with that approach?

Once you're in ~GaiaWebAuthFlow on the UI thread, you won't (or at least should
not) be able to add a reference to the destructing object to pass to the IO
thread. You'd probably want to move the IO-owned references onto a new object so
you can separate its lifetime from GaiaWebAuthFlow. I haven't thought it through
all the way, but I think once you do that life will be easier.

Maybe something like

class GaiaWebAuthFlow {
  unique_ptr<BadlyNamedGaiaWebAuthFlowIOThreadStuffClass> io_stuff_

  void CleanIOStuffFromUIThread() {
    PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&BadlyNamedGaiaWebAuthFlowIOThreadStuffClass::Cleanup,
base::Unretained(io_stuff_.release()))
  }
}

class BadlyNamedGaiaWebAuthFlowIOThreadStuffClass {
  ~BadlyNamedGaiaWebAuthFlowIOThreadStuffClass() { /* maybe complain if not on
IO thread */ }
  WeakPtr<GaiaWebAuthFlow> safe_to_post_me_to_ui_thread_tasks_ // can't be
dereferenced on IO, but that's ok
  void Cleanup() { delete this; }
}

[anthonyvd, 2016-07-14 15:22:43]

On 2016/07/13 at 21:57:53, courage wrote:
> > What do you think about making GaiaWebFlow a base::RefCountedThreadSafe (and
not
> > calling base::Unretained(this)) to take care of 1&3
> 
> Would be nice if you could avoid refcounting, but it would be ok.
> 
> > and calling
> > CleanupOnIOThread from ~GaiaWebAuthFlow to address 2? Is there anything
> > obviously wrong with that approach?
> 
> Once you're in ~GaiaWebAuthFlow on the UI thread, you won't (or at least
should not) be able to add a reference to the destructing object to pass to the
IO thread. You'd probably want to move the IO-owned references onto a new object
so you can separate its lifetime from GaiaWebAuthFlow. I haven't thought it
through all the way, but I think once you do that life will be easier.
> 
> Maybe something like
> 
> class GaiaWebAuthFlow {
>   unique_ptr<BadlyNamedGaiaWebAuthFlowIOThreadStuffClass> io_stuff_
> 
>   void CleanIOStuffFromUIThread() {
>     PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&BadlyNamedGaiaWebAuthFlowIOThreadStuffClass::Cleanup,
base::Unretained(io_stuff_.release()))
>   }
> }
> 
> class BadlyNamedGaiaWebAuthFlowIOThreadStuffClass {
>   ~BadlyNamedGaiaWebAuthFlowIOThreadStuffClass() { /* maybe complain if not on
IO thread */ }
>   WeakPtr<GaiaWebAuthFlow> safe_to_post_me_to_ui_thread_tasks_ // can't be
dereferenced on IO, but that's ok
>   void Cleanup() { delete this; }
> }

Makes a lot of sense! I've implemented something like this. Can you please take
a look?

Thanks!

[Michael Courage, 2016-07-14 17:53:31]

lgtm

https://codereview.chromium.org/2145103003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc (right):

https://codereview.chromium.org/2145103003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc:94:
content::BrowserThread::PostTask(
should check to see if io_helper_ has been set before posting. current callers
always call Start, but you never know.

[anthonyvd, 2016-07-14 18:53:45]

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

[commit-bot: I haz the power, 2016-07-14 18:54:21]

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

[anthonyvd, 2016-07-14 19:27:08]

https://codereview.chromium.org/2145103003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc (right):

https://codereview.chromium.org/2145103003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc:94:
content::BrowserThread::PostTask(
On 2016/07/14 at 17:53:30, Michael Courage wrote:
> should check to see if io_helper_ has been set before posting. current callers
always call Start, but you never know.

Done.

[anthonyvd, 2016-07-14 20:56:11]

The CQ bit was unchecked by anthonyvd@chromium.org

[anthonyvd, 2016-07-14 20:56:13]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-07-14 20:56:13]

The patchset sent to the CQ was uploaded after l-g-t-m from courage@chromium.org
Link to the patchset: https://codereview.chromium.org/2145103003/#ps60001
(title: "Only cleanup io_helper_ if it was actually created.")

[commit-bot: I haz the power, 2016-07-14 20:56:49]

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, 2016-07-14 22:11:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-14 22:11:43]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 22:14:45]

Description was changed from

==========
Request the Identity API's Uber Token without a Channel ID

BUG=621542
==========

to

==========
Request the Identity API's Uber Token without a Channel ID

BUG=621542

Committed: https://crrev.com/b07bc57f8b5e3407bc6a60901bc2da19046ac08d
Cr-Commit-Position: refs/heads/master@{#405596}
==========

[commit-bot: I haz the power, 2016-07-14 22:14:46]

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