Assign unique partition ID to gaia webview

Assign unique partition ID to gaia webview

every gaia webview should always have its own clean and isolated cookie jar. This will fix the bug that when a user signs in to chrome and then tries to add a secondary account to the profile in the same browser session, the embedded gaia webview will show a reauth page instead of a sign in page.

BUG=

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

[guohui, 2013-11-01 21:40:16]

Hey,

could you please have a look at the CL?

Thanks,

Hui

[xiyuan, 2013-11-01 21:57:11]

Can you explain a bit why this is necessary?

I think we unload gaia_auth when the InlineLoginUI is dismissed, right? Is it
because the default in-memory storage partition does not go away when gaia_auth
is unloaded and InlineLoginUI is destructed?

https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
chrome/browser/resources/gaia_auth/main.js:59: this.partitionId_ =
params.partitionId;
this.partitionId_ = params.partitionId || '';

Otherwise, I suspect we get 'undefined' as the partition id.

[guohui, 2013-11-01 22:15:14]

On 2013/11/01 21:57:11, xiyuan wrote:
> Can you explain a bit why this is necessary?
> 
> I think we unload gaia_auth when the InlineLoginUI is dismissed, right? Is it
> because the default in-memory storage partition does not go away when
gaia_auth
> is unloaded and InlineLoginUI is destructed?
Right. The default in-memory storage partition is shared by all webviews in the
same extension, and will only be destructed upon browser close.

> 
>
https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
> File chrome/browser/resources/gaia_auth/main.js (right):
> 
>
https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
> chrome/browser/resources/gaia_auth/main.js:59: this.partitionId_ =
> params.partitionId;
> this.partitionId_ = params.partitionId || '';
> 
> Otherwise, I suspect we get 'undefined' as the partition id.

[guohui, 2013-11-01 22:16:05]

https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/45463004/diff/1/chrome/browser/resources/gaia...
chrome/browser/resources/gaia_auth/main.js:59: this.partitionId_ =
params.partitionId;
On 2013/11/01 21:57:11, xiyuan wrote:
> this.partitionId_ = params.partitionId || '';
> 
> Otherwise, I suspect we get 'undefined' as the partition id.

Done.

[xiyuan, 2013-11-01 22:30:44]

On 2013/11/01 22:15:14, guohui wrote:
> On 2013/11/01 21:57:11, xiyuan wrote:
> > Can you explain a bit why this is necessary?
> > 
> > I think we unload gaia_auth when the InlineLoginUI is dismissed, right? Is
it
> > because the default in-memory storage partition does not go away when
> gaia_auth
> > is unloaded and InlineLoginUI is destructed?
> Right. The default in-memory storage partition is shared by all webviews in
the
> same extension, and will only be destructed upon browser close.

Any chance to change so that the default in-memory storage partition is cleaned
up when the extension is unloaded?

[Fady Samuel, 2013-11-04 00:23:02]

Istiaque has done some work on StoragePartitions. Thoughts? Thanks! :-)

[lazyboy, 2013-11-04 21:08:28]

Any chance to change so that the default in-memory storage partition is cleaned
up when the extension is unloaded?

That might affect all the in-memory data in that partition, which other
<webview> might be using, which isn't desirable?

The ClearData* api might be used to clear cookies on a <webview>, but that also
have the same problem, it will clear all the cookies in default in_memory
partition. Other issue I see is when you're going to clear the data?, you have
to load a <webview> before clearing its data. So if we have a function that gets
called when the <webview> is going away, you might want to call ClearData() from
there.

*https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/extensions/platform_apps/web_view/common/cleardata/bootstrap.js&l=40

https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/res...
File chrome/browser/resources/gaia_auth/main.js (right):

https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/res...
chrome/browser/resources/gaia_auth/main.js:132: gaiaFrame.partition =
this.partitionId_;
I'm wondering since partitionId_ is an increasing sequence, could we end up in a
case where:
You load account A in a webview
then load account B in webview
and then load account A again
Would the third load have different partitionId_ than first load? I'm not
entirely sure if this is a possible scenario though. If it is possible, would
that be a problem that account A has two different storage?

https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/ui/...
File chrome/browser/ui/webui/inline_login_ui.cc (right):

https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/ui/...
chrome/browser/ui/webui/inline_login_ui.cc:212: partition_id_));
nit: generally query params have "query=value" form, so
"?partition=gaia-webview-1" is probably better? This might become an issue if we
need to add more query parameters to the GURL later.

[guohui, 2013-11-05 14:29:59]

Thanks for all your input! While i agree it is a nice feature request for
webview to support fully isolated default storage, i think it is a separate
issue. Given that this CL is blocking our demo on All Hands next Tuesday, I
would appreciate it a lot if we could address this issue in a separate thread.

On 2013/11/04 21:08:28, lazyboy wrote:
> Any chance to change so that the default in-memory storage partition is
cleaned
> up when the extension is unloaded?
> 
> That might affect all the in-memory data in that partition, which other
> <webview> might be using, which isn't desirable?
Agree. If the default storage is gonna clear itself up upon unload, then i think
we must ensure each webview instance has its own default storage to avoid
interference.

> 
> The ClearData* api might be used to clear cookies on a <webview>, but that
also
> have the same problem, it will clear all the cookies in default in_memory
> partition. Other issue I see is when you're going to clear the data?, you have
> to load a <webview> before clearing its data. So if we have a function that
gets
> called when the <webview> is going away, you might want to call ClearData()
from
> there.
> 
>
*https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/extensions/platform_apps/web_view/common/cleardata/bootstrap.js&l=40
> 
>
https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/res...
> File chrome/browser/resources/gaia_auth/main.js (right):
> 
>
https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/res...
> chrome/browser/resources/gaia_auth/main.js:132: gaiaFrame.partition =
> this.partitionId_;
> I'm wondering since partitionId_ is an increasing sequence, could we end up in
a
> case where:
> You load account A in a webview
> then load account B in webview
> and then load account A again
> Would the third load have different partitionId_ than first load? I'm not
> entirely sure if this is a possible scenario though. If it is possible, would
> that be a problem that account A has two different storage?
> 
>
https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/ui/...
> File chrome/browser/ui/webui/inline_login_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/45463004/diff/70001/chrome/browser/ui/...
> chrome/browser/ui/webui/inline_login_ui.cc:212: partition_id_));
> nit: generally query params have "query=value" form, so
> "?partition=gaia-webview-1" is probably better? This might become an issue if
we
> need to add more query parameters to the GURL later.

[guohui, 2013-11-05 14:35:35]

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/resources/...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/resources/...
chrome/browser/resources/gaia_auth/main.js:132: gaiaFrame.partition =
this.partitionId_;
On 2013/11/04 21:08:29, lazyboy wrote:
> I'm wondering since partitionId_ is an increasing sequence, could we end up in
a
> case where:
> You load account A in a webview
> then load account B in webview
> and then load account A again
> Would the third load have different partitionId_ than first load? I'm not
> entirely sure if this is a possible scenario though. If it is possible, would
> that be a problem that account A has two different storage?
this is ok. In our use case, we always want a clean cookie jar, regardless of
which account the user picks.

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/ui/webui/i...
File chrome/browser/ui/webui/inline_login_ui.cc (right):

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/ui/webui/i...
chrome/browser/ui/webui/inline_login_ui.cc:212: partition_id_));
On 2013/11/04 21:08:29, lazyboy wrote:
> nit: generally query params have "query=value" form, so
> "?partition=gaia-webview-1" is probably better? This might become an issue if
we
> need to add more query parameters to the GURL later.
i agree, but i thought currently ?partition_id is the required format for
webview guest site instance.
@fady, could you please confirm?

[xiyuan, 2013-11-05 17:49:22]

On 2013/11/05 14:29:59, guohui wrote:
> Thanks for all your input! While i agree it is a nice feature request for
> webview to support fully isolated default storage, i think it is a separate
> issue. Given that this CL is blocking our demo on All Hands next Tuesday, I
> would appreciate it a lot if we could address this issue in a separate thread.
> 

My biggest concern is that we create many one time storage partitions in this
CL. When do we release them? Earlier you mentioned that they are only released
on browser close. Would that be a problem?

[guohui, 2013-11-05 19:57:06]

On 2013/11/05 17:49:22, xiyuan wrote:
> On 2013/11/05 14:29:59, guohui wrote:
> > Thanks for all your input! While i agree it is a nice feature request for
> > webview to support fully isolated default storage, i think it is a separate
> > issue. Given that this CL is blocking our demo on All Hands next Tuesday, I
> > would appreciate it a lot if we could address this issue in a separate
thread.
> > 
> 
> My biggest concern is that we create many one time storage partitions in this
> CL. When do we release them? Earlier you mentioned that they are only released
> on browser close. Would that be a problem?

the gaia webview is only loaded when user clicks "sign in" in a disconnected
profile or "add account" in a connected profile(except for the case when user
explicitly types the URL in address bar). Since most users have at most one
account per profile, it means for most users the CL will create one partition in
a disconnected session and 0 in a connected session. It is true that if user
aborts signin and then try again within the same session, a second or third
partition will be created, but it should be rare case, and even in those cases,
all temporary partitions will be cleared upon browser close. Also AFAIK
in-memory storage partition is quite light-weight object that should not have
any noticable impact on performance.
@Fady & Istiaque, could you please confirm this?

[Fady Samuel, 2013-11-05 19:59:50]

On 2013/11/05 19:57:06, guohui wrote:
> On 2013/11/05 17:49:22, xiyuan wrote:
> > On 2013/11/05 14:29:59, guohui wrote:
> > > Thanks for all your input! While i agree it is a nice feature request for
> > > webview to support fully isolated default storage, i think it is a
separate
> > > issue. Given that this CL is blocking our demo on All Hands next Tuesday,
I
> > > would appreciate it a lot if we could address this issue in a separate
> thread.
> > > 
> > 
> > My biggest concern is that we create many one time storage partitions in
this
> > CL. When do we release them? Earlier you mentioned that they are only
released
> > on browser close. Would that be a problem?
> 
> the gaia webview is only loaded when user clicks "sign in" in a disconnected
> profile or "add account" in a connected profile(except for the case when user
> explicitly types the URL in address bar). Since most users have at most one
> account per profile, it means for most users the CL will create one partition
in
> a disconnected session and 0 in a connected session. It is true that if user
> aborts signin and then try again within the same session, a second or third
> partition will be created, but it should be rare case, and even in those
cases,
> all temporary partitions will be cleared upon browser close. Also AFAIK
> in-memory storage partition is quite light-weight object that should not have
> any noticable impact on performance.
> @Fady & Istiaque, could you please confirm this?

As far as as I'm aware (we didn't write StoragePartitions, ajwong@ and nasko@
did), they are fairly lightweight. It might be useful to file a bug to
investigate better cleanup mechanisms for storage partitions.

[xiyuan, 2013-11-05 20:39:54]

Thanks for the explanation. It still feels wrong to keep
never-going-to-be-used-again objects around indefinitely. But I agree it is
probably out of the scope of this CL. Let's file a bug as Fady suggested for
better cleanup mechanisms and a TODO to revisit this after that bug is resolved.

LGTM

[guohui, 2013-11-05 20:43:43]

On 2013/11/05 20:39:54, xiyuan wrote:
> Thanks for the explanation. It still feels wrong to keep
> never-going-to-be-used-again objects around indefinitely. But I agree it is
> probably out of the scope of this CL. Let's file a bug as Fady suggested for
> better cleanup mechanisms and a TODO to revisit this after that bug is
resolved.
> 
> LGTM

thanks xiyuan! filed crbug/315230.

[Fady Samuel, 2013-11-05 21:47:07]

lgtm

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/ui/webui/i...
File chrome/browser/ui/webui/inline_login_ui.cc (right):

https://codereview.chromium.org/45463004/diff/70001/chrome/browser/ui/webui/i...
chrome/browser/ui/webui/inline_login_ui.cc:212: partition_id_));
On 2013/11/05 14:35:36, guohui wrote:
> On 2013/11/04 21:08:29, lazyboy wrote:
> > nit: generally query params have "query=value" form, so
> > "?partition=gaia-webview-1" is probably better? This might become an issue
if
> we
> > need to add more query parameters to the GURL later.
> i agree, but i thought currently ?partition_id is the required format for
> webview guest site instance.
> @fady, could you please confirm?

Yes, Hui is correct. See here: 

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

[commit-bot: I haz the power, 2013-11-05 22:15:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/45463004/70001

[commit-bot: I haz the power, 2013-11-05 23:29:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-06 03:11:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/45463004/70001

[commit-bot: I haz the power, 2013-11-06 03:55:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[xiyuan, 2013-11-06 04:38:17]

On 2013/11/06 03:55:44, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on linux_chromeos.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

FAILED: g++ -MMD -MF obj/chrome/browser/ui/webui/browser_ui.inline_login_ui.o.d
... 
../../chrome/browser/ui/webui/inline_login_ui.cc:97:41:error:
'{anonymous}::next_partition_id' defined but not used [-Werror=unused-variable]
cc1plus: all warnings being treated as errors

Looks real. next_partition_id is unused on chromeos.

[guohui, 2013-11-06 19:26:01]

On 2013/11/06 04:38:17, xiyuan wrote:
> On 2013/11/06 03:55:44, I haz the power (commit-bot) wrote:
> > Sorry for I got bad news for ya.
> > Compile failed with a clobber build on linux_chromeos.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> > Your code is likely broken or HEAD is junk. Please ensure your
> > code is not broken then alert the build sheriffs.
> > Look at the try server FAQ for more details.
> 
> FAILED: g++ -MMD -MF
obj/chrome/browser/ui/webui/browser_ui.inline_login_ui.o.d
> ... 
> ../../chrome/browser/ui/webui/inline_login_ui.cc:97:41:error:
> '{anonymous}::next_partition_id' defined but not used
[-Werror=unused-variable]
> cc1plus: all warnings being treated as errors
> 
> Looks real. next_partition_id is unused on chromeos.

thanks, fixed in the new patch.

[commit-bot: I haz the power, 2013-11-06 19:29:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/45463004/540001

[commit-bot: I haz the power, 2013-11-06 21:10:51]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-06 22:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/45463004/540001

[commit-bot: I haz the power, 2013-11-07 01:17:16]

Change committed as 233456