Don't use request user data during one-click sign in

When the user says "don't show me again", make sure to respect it.

In some cases, its not possible to maintain context information from http
requests using URLRequest user data.  Redirects during the sign in process
may use 200 OK responses with html that causes an auto refresh.  Use the
io profile data instead to hold the state.

BUG=169874
TEST=See bug description for repro steps


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178947

[erikwright (departed), 2013-01-18 03:06:54]

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:681:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
This is a bit of an abuse (perhaps harmless) of Preferences.

And in any case, it's not really necessary - presumably you can store this data
in ShowInfoBarIfPossible, in which case you save yourself all the trouble of
moving this data back and forth across threads (and cleaning it up afterwards).

[Roger Tawa OOO till Jul 10th, 2013-01-18 15:21:51]

Thanks Erik.  See question below.

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:681:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
On 2013/01/18 03:06:54, erikwright wrote:
> This is a bit of an abuse (perhaps harmless) of Preferences.
> 
> And in any case, it's not really necessary - presumably you can store this
data
> in ShowInfoBarIfPossible, in which case you save yourself all the trouble of
> moving this data back and forth across threads (and cleaning it up
afterwards).

Are you suggesting I use an std::string member directly in ProfileIOData, which
can be set in ShowInfoBarIfPossible() and read in CanOfferOnIOThreadImpl(),
since both occur on the IO thread?

However, I still need to be able to clear the value when the process is done,
see line 744 below, and this happens on the UI thread.  Maybe I can post to the
IO thread at line 744 to clear the value?

[erikwright (departed), 2013-01-18 15:36:28]

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:681:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
On 2013/01/18 15:21:51, Roger Tawa wrote:
> On 2013/01/18 03:06:54, erikwright wrote:
> > This is a bit of an abuse (perhaps harmless) of Preferences.
> > 
> > And in any case, it's not really necessary - presumably you can store this
> data
> > in ShowInfoBarIfPossible, in which case you save yourself all the trouble of
> > moving this data back and forth across threads (and cleaning it up
> afterwards).
> 
> Are you suggesting I use an std::string member directly in ProfileIOData,
which
> can be set in ShowInfoBarIfPossible() and read in CanOfferOnIOThreadImpl(),
> since both occur on the IO thread?
> 
> However, I still need to be able to clear the value when the process is done,
> see line 744 below, and this happens on the UI thread.  Maybe I can post to
the
> IO thread at line 744 to clear the value?

That's basically what I'm suggesting. Here are a few reasons:

1) There are other side effects of using preferences (i.e., this value will be
persisted if the browser quits/crashes during the process, no?) that it would be
best to avoid.
2) The cross-thread marshalling will be explicit, clear, and correlated with the
movement of the logic across threads.
3) I'm concerned about possible surprises - what if the tasks that are updating
the pref member and the tasks that are reading it don't get interleaved exactly
as you expect? You might end up reading a stale value.

Mostly, it just feels like it is unnecessary and a little abusive. #3 is
probably not a problem in practice.

So I'd suggest giving it a try and seeing what the impact on the code is.

[Roger Tawa OOO till Jul 10th, 2013-01-18 18:57:43]

Hi Erik, please take a look.  Is there a chance that the resource context could
go away between the post at line 754 and the invocation at line 126?  If so,
what is the best way to handle that?

[erikwright (departed), 2013-01-18 19:06:59]

On 2013/01/18 18:57:43, Roger Tawa wrote:
> Hi Erik, please take a look.  Is there a chance that the resource context
could
> go away between the post at line 754 and the invocation at line 126?  If so,
> what is the best way to handle that?

Looks right to me.

ResourceContext is owned by the profile, and should be destroyed on the IO
thread during profile shutdown. Assuming that your code is not running in the
late phases of profile shutdown I think you are good.

[Roger Tawa OOO till Jul 10th, 2013-01-18 22:17:30]

Hi Matt, Scott,

Can you please review?

Matt: can you look at the profile io and resource dispatch host delegate
changes?

Scott: can you look at the other changes?

Thanks.

[mmenke, 2013-01-18 22:35:29]

"nit" should be "not" in your description.

I'm not an owner of chrome_resource_dispatcher_host_delegate.cc, but the profile
changes LGTM.

https://codereview.chromium.org/11938007/diff/3002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11938007/diff/3002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:422: std::string
reverse_autologin_pending_email_;
I think this name is sufficiently obscure to be worth a short comment.

[sky, 2013-01-18 23:11:43]

I'm a bit confused as to which files you need to review. Not the sync ones,
right?

[Roger Tawa OOO till Jul 10th, 2013-01-21 15:26:45]

Thanks Matt.

Scott: You're right, I think I only need an owner for
chrome_resource_dispatcher_host_delegate.cc, but feel free to comment elsewhere
too.  I'll get someone from sync to review one click.  Thanks.

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:681:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
On 2013/01/18 15:36:28, erikwright wrote:
> On 2013/01/18 15:21:51, Roger Tawa wrote:
> > On 2013/01/18 03:06:54, erikwright wrote:
> > > This is a bit of an abuse (perhaps harmless) of Preferences.
> > > 
> > > And in any case, it's not really necessary - presumably you can store this
> > data
> > > in ShowInfoBarIfPossible, in which case you save yourself all the trouble
of
> > > moving this data back and forth across threads (and cleaning it up
> > afterwards).
> > 
> > Are you suggesting I use an std::string member directly in ProfileIOData,
> which
> > can be set in ShowInfoBarIfPossible() and read in CanOfferOnIOThreadImpl(),
> > since both occur on the IO thread?
> > 
> > However, I still need to be able to clear the value when the process is
done,
> > see line 744 below, and this happens on the UI thread.  Maybe I can post to
> the
> > IO thread at line 744 to clear the value?
> 
> That's basically what I'm suggesting. Here are a few reasons:
> 
> 1) There are other side effects of using preferences (i.e., this value will be
> persisted if the browser quits/crashes during the process, no?) that it would
be
> best to avoid.
> 2) The cross-thread marshalling will be explicit, clear, and correlated with
the
> movement of the logic across threads.
> 3) I'm concerned about possible surprises - what if the tasks that are
updating
> the pref member and the tasks that are reading it don't get interleaved
exactly
> as you expect? You might end up reading a stale value.
> 
> Mostly, it just feels like it is unnecessary and a little abusive. #3 is
> probably not a problem in practice.
> 
> So I'd suggest giving it a try and seeing what the impact on the code is.

Done.

https://codereview.chromium.org/11938007/diff/3002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11938007/diff/3002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:422: std::string
reverse_autologin_pending_email_;
On 2013/01/18 22:35:29, Matt Menke wrote:
> I think this name is sufficiently obscure to be worth a short comment.

Done.

[Roger Tawa OOO till Jul 10th, 2013-01-21 15:28:28]

Hi Tim,

Can you please take a look?  Thanks.

[sky, 2013-01-22 16:25:41]

LGTM

[tim (not reviewing), 2013-01-23 02:07:52]

A few style nits, then LGTM.

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:435: // During the reverse autologin
request chain processing, this member save
nit - saves

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:129: if (io_data)
Is there a reason you're handling this this way?  Convention would say instead
of DCHECK(ptr) if (ptr), you would just do ptr->blah(), since the way it is
written now seems to imply that the DCHECK can in fact fail, which is
contradictory.  

See the 'Coding Style' section on dev.chromium.org, specifically the following
passage - "... A consequence of this is that you should not handle DCHECK()
failures, even if failure would result in a crash. Attempting to handle a
DCHECK() failure is a statement that the DCHECK() can fail, which contradicts
the point of writing the DCHECK()."

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:130:
io_data->set_reverse_autologin_pending_email("");
nit - prefer std::string() to "", as it is more explicit / clear, and more
efficient.

[Roger Tawa OOO till Jul 10th, 2013-01-25 20:14:10]

Thanks Tim.  Changes uploaded.

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:435: // During the reverse autologin
request chain processing, this member save
On 2013/01/23 02:07:52, timsteele wrote:
> nit - saves

Done.

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:129: if (io_data)
On 2013/01/23 02:07:52, timsteele wrote:
> Is there a reason you're handling this this way?  Convention would say instead
> of DCHECK(ptr) if (ptr), you would just do ptr->blah(), since the way it is
> written now seems to imply that the DCHECK can in fact fail, which is
> contradictory.  
> 
> See the 'Coding Style' section on http://dev.chromium.org, specifically the
following
> passage - "... A consequence of this is that you should not handle DCHECK()
> failures, even if failure would result in a crash. Attempting to handle a
> DCHECK() failure is a statement that the DCHECK() can fail, which contradicts
> the point of writing the DCHECK()."

Done.

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:130:
io_data->set_reverse_autologin_pending_email("");
On 2013/01/23 02:07:52, timsteele wrote:
> nit - prefer std::string() to "", as it is more explicit / clear, and more
> efficient.

Done.

https://codereview.chromium.org/11938007/diff/21002/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:130:
io_data->set_reverse_autologin_pending_email("");
On 2013/01/23 02:07:52, timsteele wrote:
> nit - prefer std::string() to "", as it is more explicit / clear, and more
> efficient.

Done.

[commit-bot: I haz the power, 2013-01-25 20:18:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/11938007/25003

[commit-bot: I haz the power, 2013-01-26 00:07:08]

Change committed as 178947