WebUI: shuffle less between threads when responding to requests

WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127
Committed: https://crrev.com/78b98db5654803262d752a07050e7d77f6cc6bfa
Cr-Commit-Position: refs/heads/master@{#394944}

[Dan Beam, 2016-05-17 03:06:17]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which didn't
seem to actually do anything.

R=jam@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just seemed
to be dead code for the last 2.2 years.

R=jam@chromium.org
BUG=none
==========

[Dan Beam, 2016-05-17 03:06:48]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just seemed
to be dead code for the last 2.2 years.

R=jam@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=jam@chromium.org
BUG=none
==========

[Dan Beam, 2016-05-17 03:24:12]

dbeam@chromium.org changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[Dan Beam, 2016-05-17 03:24:14]

jam@ seems offsite or OOO for a while, so the `git cl owners` roulette wheel
lands on.... avi@!

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (left):

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:404: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
|hosts| was never used because

  if (thing && (thing || other_thing))

never hits other_thing. this check was effectively:

  if (url.SchemeIs(kChromeUIScheme)) {

which can be performed synchronously from the IO thread without posting to UI
thread and back.  same with the PID check.

[Avi (use Gerrit), 2016-05-17 03:30:20]

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (left):

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:404: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
On 2016/05/17 03:24:14, Dan Beam wrote:
> |hosts| was never used because
>   if (thing && (thing || other_thing))
> never hits other_thing.

Right, but what CL introduced this silliness? What were they intending to do?
Were what they were worrying about something we should fix rather than just rip
out?

[Dan Beam, 2016-05-17 03:32:16]

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (left):

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:404: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
On 2016/05/17 03:30:20, Avi wrote:
> On 2016/05/17 03:24:14, Dan Beam wrote:
> > |hosts| was never used because
> >   if (thing && (thing || other_thing))
> > never hits other_thing.
> 
> Right, but what CL introduced this silliness? What were they intending to do?
> Were what they were worrying about something we should fix rather than just
rip
> out?

https://codereview.chromium.org/183803023/#msg10

[Dan Beam, 2016-05-17 03:39:03]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=jam@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

[jam, 2016-05-17 19:13:43]

btw I don't remember the details, but this was related for webview-based signin.

It's probably worth it to check, perhaps with nasko or creis, to know if there
are any security implications with the original intent.

[Dan Beam, 2016-05-17 19:29:14]

/cc nasko@, creis@

[groby-ooo-7-16, 2016-05-17 20:33:46]

On 2016/05/17 19:29:14, Dan Beam wrote:
> /cc nasko@, creis@

For a bit more context: Yes, this is related to assigning a separate partition
map to chrome://chrome-signin, as per the original CL[1]. But as Dan pointed
out, this code has been broken for two years, in a "cleanup" CL [2][3] - the new
code preserves existing behavior.

If we _were_ to fix this, I'm not even sure the request job is the right place.
At the very least, this should probably bubble up to
ChromeProtocolHandler::MaybeCreateJob.

[1]
https://chromium.googlesource.com/chromium/src/+/4413d87abb4483aecd867cac1e07...
[2] https://codereview.chromium.org/183803023
[3] Sometimes I wish we disallowed CLs without attached bugs. Then I lie down,
and it passes ;)

[Dan Beam, 2016-05-17 21:03:47]

On 2016/05/17 20:33:46, groby wrote:
> On 2016/05/17 19:29:14, Dan Beam wrote:
> > /cc nasko@, creis@
> 
> For a bit more context: Yes, this is related to assigning a separate partition
> map to chrome://chrome-signin, as per the original CL[1]. But as Dan pointed
> out, this code has been broken for two years, in a "cleanup" CL [2][3] - the
new
> code preserves existing behavior.
> 
> If we _were_ to fix this, I'm not even sure the request job is the right
place.
> At the very least, this should probably bubble up to
> ChromeProtocolHandler::MaybeCreateJob.

And as groby@ astutely pointed out to me on chat: all URLs are chrome:// scheme
by the time they hit this code, so we could probably remove more code if we'd
like.

> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/4413d87abb4483aecd867cac1e07...
> [2] https://codereview.chromium.org/183803023
> [3] Sometimes I wish we disallowed CLs without attached bugs. Then I lie down,
> and it passes ;)

[Charlie Reis, 2016-05-17 21:08:10]

creis@chromium.org changed reviewers:
+ creis@chromium.org, rogerta@chromium.org

[Charlie Reis, 2016-05-17 21:08:10]

[+rogerta]

Wow, thanks for coming across this.  It looks bad, but maybe it's all just dead
code.

Roger, can you confirm if this whole thing can be removed as a relic of
iframe-based signin?

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
File content/browser/webui/url_data_manager_backend.cc (left):

https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
content/browser/webui/url_data_manager_backend.cc:404: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
On 2016/05/17 03:32:16, Dan Beam wrote:
> On 2016/05/17 03:30:20, Avi wrote:
> > On 2016/05/17 03:24:14, Dan Beam wrote:
> > > |hosts| was never used because
> > >   if (thing && (thing || other_thing))
> > > never hits other_thing.
> > 
> > Right, but what CL introduced this silliness? What were they intending to
do?
> > Were what they were worrying about something we should fix rather than just
> rip
> > out?
> 
> https://codereview.chromium.org/183803023/#msg10

Yeah, this code is definitely broken after
https://codereview.chromium.org/183803023/.

That was a refactor to https://codereview.chromium.org/130963006, which moved
chrome://chrome-signin from a webview to an iframe.

Looks like the intent was to make sure that only whitelisted WebUI hosts could
get past the StoragePartition check, but after the refactor *any* chrome:// URL
could (which meant powerful pages like chrome://settings, etc).  That kind of
defeated the point of the check.  (I'm not sure how we were able to have an
iframe with a different StoragePartition in the first place-- I don't understand
that part of the original CL's description.)

However, if I understand correctly, we have since moved back to the webview
approach for sign-in, and the iframe approach is no longer used.  And yes, the
old iframe based approach looks like it was removed in rogerta's
https://codereview.chromium.org/1412453002.

I'm not sure why this code wasn't removed along with the rest-- hopefully it
isn't necessary to load any WebUI pages (within the host whitelist or not)
inside the webview, so this check shouldn't be needed.  Maybe the entire
CheckStoragePartitionMatches function can go away, since it was only introduced
for the iframe version?  (Indeed, it seems a bit unsafe to leave it here,
certainly as written.  It looks like we block navigating the webview to
chrome:// URLs in other ways when I try it out, but it feels risky to have this
path around.)

Roger, can you confirm whether this has any value now?

[Roger Tawa OOO till Jul 10th, 2016-05-18 20:00:16]

On 2016/05/17 at 21:08:10, creis wrote:
> [+rogerta]
> 
> Wow, thanks for coming across this.  It looks bad, but maybe it's all just
dead code.
> 
> Roger, can you confirm if this whole thing can be removed as a relic of
iframe-based signin?
> 
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> File content/browser/webui/url_data_manager_backend.cc (left):
> 
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> content/browser/webui/url_data_manager_backend.cc:404:
std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end())) {
> On 2016/05/17 03:32:16, Dan Beam wrote:
> > On 2016/05/17 03:30:20, Avi wrote:
> > > On 2016/05/17 03:24:14, Dan Beam wrote:
> > > > |hosts| was never used because
> > > >   if (thing && (thing || other_thing))
> > > > never hits other_thing.
> > > 
> > > Right, but what CL introduced this silliness? What were they intending to
do?
> > > Were what they were worrying about something we should fix rather than
just
> > rip
> > > out?
> > 
> > https://codereview.chromium.org/183803023/#msg10
> 
> Yeah, this code is definitely broken after
https://codereview.chromium.org/183803023/.
> 
> That was a refactor to https://codereview.chromium.org/130963006, which moved
chrome://chrome-signin from a webview to an iframe.
> 
> Looks like the intent was to make sure that only whitelisted WebUI hosts could
get past the StoragePartition check, but after the refactor *any* chrome:// URL
could (which meant powerful pages like chrome://settings, etc).  That kind of
defeated the point of the check.  (I'm not sure how we were able to have an
iframe with a different StoragePartition in the first place-- I don't understand
that part of the original CL's description.)
> 
> However, if I understand correctly, we have since moved back to the webview
approach for sign-in, and the iframe approach is no longer used.  And yes, the
old iframe based approach looks like it was removed in rogerta's
https://codereview.chromium.org/1412453002.
> 
> I'm not sure why this code wasn't removed along with the rest-- hopefully it
isn't necessary to load any WebUI pages (within the host whitelist or not)
inside the webview, so this check shouldn't be needed.  Maybe the entire
CheckStoragePartitionMatches function can go away, since it was only introduced
for the iframe version?  (Indeed, it seems a bit unsafe to leave it here,
certainly as written.  It looks like we block navigating the webview to
chrome:// URLs in other ways when I try it out, but it feels risky to have this
path around.)
> 
> Roger, can you confirm whether this has any value now?

Hi Charlie,

The iframe based sign in mechanism is gone.  I am not aware of this code and
maybe should have deleted it with my CL.  I'm not working on chrome sign-in any
more and this is all a while back.  I don't remember why these exceptions were
needed.  The best way to check if the code is still required: when performing a
sign in to chrome:

1/ make sure the renderer process used to show the gaia sign in page is not
shared with any other existing renderer process
2/ make sure that the cookie jar of that gaia page is not shared with the
profile's main cookie jar
3/ make sure sign in to chrome works

[Charlie Reis, 2016-05-18 20:03:57]

On 2016/05/18 20:00:16, Roger Tawa wrote:
> On 2016/05/17 at 21:08:10, creis wrote:
> > [+rogerta]
> > 
> > Wow, thanks for coming across this.  It looks bad, but maybe it's all just
> dead code.
> > 
> > Roger, can you confirm if this whole thing can be removed as a relic of
> iframe-based signin?
> > 
> >
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > File content/browser/webui/url_data_manager_backend.cc (left):
> > 
> >
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > content/browser/webui/url_data_manager_backend.cc:404:
> std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end())) {
> > On 2016/05/17 03:32:16, Dan Beam wrote:
> > > On 2016/05/17 03:30:20, Avi wrote:
> > > > On 2016/05/17 03:24:14, Dan Beam wrote:
> > > > > |hosts| was never used because
> > > > >   if (thing && (thing || other_thing))
> > > > > never hits other_thing.
> > > > 
> > > > Right, but what CL introduced this silliness? What were they intending
to
> do?
> > > > Were what they were worrying about something we should fix rather than
> just
> > > rip
> > > > out?
> > > 
> > > https://codereview.chromium.org/183803023/#msg10
> > 
> > Yeah, this code is definitely broken after
> https://codereview.chromium.org/183803023/.
> > 
> > That was a refactor to https://codereview.chromium.org/130963006, which
moved
> chrome://chrome-signin from a webview to an iframe.
> > 
> > Looks like the intent was to make sure that only whitelisted WebUI hosts
could
> get past the StoragePartition check, but after the refactor *any* chrome://
URL
> could (which meant powerful pages like chrome://settings, etc).  That kind of
> defeated the point of the check.  (I'm not sure how we were able to have an
> iframe with a different StoragePartition in the first place-- I don't
understand
> that part of the original CL's description.)
> > 
> > However, if I understand correctly, we have since moved back to the webview
> approach for sign-in, and the iframe approach is no longer used.  And yes, the
> old iframe based approach looks like it was removed in rogerta's
> https://codereview.chromium.org/1412453002.
> > 
> > I'm not sure why this code wasn't removed along with the rest-- hopefully it
> isn't necessary to load any WebUI pages (within the host whitelist or not)
> inside the webview, so this check shouldn't be needed.  Maybe the entire
> CheckStoragePartitionMatches function can go away, since it was only
introduced
> for the iframe version?  (Indeed, it seems a bit unsafe to leave it here,
> certainly as written.  It looks like we block navigating the webview to
> chrome:// URLs in other ways when I try it out, but it feels risky to have
this
> path around.)
> > 
> > Roger, can you confirm whether this has any value now?
> 
> Hi Charlie,
> 
> The iframe based sign in mechanism is gone.  I am not aware of this code and
> maybe should have deleted it with my CL.  I'm not working on chrome sign-in
any
> more and this is all a while back.  I don't remember why these exceptions were
> needed.  The best way to check if the code is still required: when performing
a
> sign in to chrome:
> 
> 1/ make sure the renderer process used to show the gaia sign in page is not
> shared with any other existing renderer process
> 2/ make sure that the cookie jar of that gaia page is not shared with the
> profile's main cookie jar
> 3/ make sure sign in to chrome works

Thanks.  There's tests for these, right?  Maybe dbeam can try removing this code
entirely and seeing if the tests are happy?

[Roger Tawa OOO till Jul 10th, 2016-05-18 20:15:02]

+Anthony,

Anthony has redone the sign-in flow for gaia's new password separated flow,
so may he can mention where the tests are.

Thanks,
Roger

-

On Wed, May 18, 2016 at 4:03 PM, <creis@chromium.org> wrote:

> On 2016/05/18 20:00:16, Roger Tawa wrote:
> > On 2016/05/17 at 21:08:10, creis wrote:
> > > [+rogerta]
> > >
> > > Wow, thanks for coming across this. It looks bad, but maybe it's all
> just
> > dead code.
> > >
> > > Roger, can you confirm if this whole thing can be removed as a relic of
> > iframe-based signin?
> > >
> > >
> >
>
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > > File content/browser/webui/url_data_manager_backend.cc (left):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > > content/browser/webui/url_data_manager_backend.cc:404:
> > std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end())) {
> > > On 2016/05/17 03:32:16, Dan Beam wrote:
> > > > On 2016/05/17 03:30:20, Avi wrote:
> > > > > On 2016/05/17 03:24:14, Dan Beam wrote:
> > > > > > |hosts| was never used because
> > > > > > if (thing && (thing || other_thing))
> > > > > > never hits other_thing.
> > > > >
> > > > > Right, but what CL introduced this silliness? What were they
> intending
> to
> > do?
> > > > > Were what they were worrying about something we should fix rather
> than
> > just
> > > > rip
> > > > > out?
> > > >
> > > > https://codereview.chromium.org/183803023/#msg10
> > >
> > > Yeah, this code is definitely broken after
> > https://codereview.chromium.org/183803023/.
> > >
> > > That was a refactor to https://codereview.chromium.org/130963006,
> which
> moved
> > chrome://chrome-signin from a webview to an iframe.
> > >
> > > Looks like the intent was to make sure that only whitelisted WebUI
> hosts
> could
> > get past the StoragePartition check, but after the refactor *any*
> chrome://
> URL
> > could (which meant powerful pages like chrome://settings, etc). That
> kind of
> > defeated the point of the check. (I'm not sure how we were able to have
> an
> > iframe with a different StoragePartition in the first place-- I don't
> understand
> > that part of the original CL's description.)
> > >
> > > However, if I understand correctly, we have since moved back to the
> webview
> > approach for sign-in, and the iframe approach is no longer used. And
> yes, the
> > old iframe based approach looks like it was removed in rogerta's
> > https://codereview.chromium.org/1412453002.
> > >
> > > I'm not sure why this code wasn't removed along with the rest--
> hopefully it
> > isn't necessary to load any WebUI pages (within the host whitelist or
> not)
> > inside the webview, so this check shouldn't be needed. Maybe the entire
> > CheckStoragePartitionMatches function can go away, since it was only
> introduced
> > for the iframe version? (Indeed, it seems a bit unsafe to leave it here,
> > certainly as written. It looks like we block navigating the webview to
> > chrome:// URLs in other ways when I try it out, but it feels risky to
> have
> this
> > path around.)
> > >
> > > Roger, can you confirm whether this has any value now?
> >
> > Hi Charlie,
> >
> > The iframe based sign in mechanism is gone. I am not aware of this code
> and
> > maybe should have deleted it with my CL. I'm not working on chrome
> sign-in
> any
> > more and this is all a while back. I don't remember why these exceptions
> were
> > needed. The best way to check if the code is still required: when
> performing
> a
> > sign in to chrome:
> >
> > 1/ make sure the renderer process used to show the gaia sign in page is
> not
> > shared with any other existing renderer process
> > 2/ make sure that the cookie jar of that gaia page is not shared with the
> > profile's main cookie jar
> > 3/ make sure sign in to chrome works
>
> Thanks. There's tests for these, right? Maybe dbeam can try removing this
> code
> entirely and seeing if the tests are happy?
>
> https://codereview.chromium.org/1985983002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[anthonyvd, 2016-05-18 20:47:46]

On 2016/05/18 at 20:15:02, rogerta wrote:
> +Anthony,
> 
> Anthony has redone the sign-in flow for gaia's new password separated flow,
> so may he can mention where the tests are.
> 
> Thanks,
> Roger
> 
> -
> 
> On Wed, May 18, 2016 at 4:03 PM, <creis@chromium.org> wrote:
> 
> > On 2016/05/18 20:00:16, Roger Tawa wrote:
> > > On 2016/05/17 at 21:08:10, creis wrote:
> > > > [+rogerta]
> > > >
> > > > Wow, thanks for coming across this. It looks bad, but maybe it's all
> > just
> > > dead code.
> > > >
> > > > Roger, can you confirm if this whole thing can be removed as a relic of
> > > iframe-based signin?
> > > >
> > > >
> > >
> >
> >
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > > > File content/browser/webui/url_data_manager_backend.cc (left):
> > > >
> > > >
> > >
> >
> >
https://codereview.chromium.org/1985983002/diff/1/content/browser/webui/url_d...
> > > > content/browser/webui/url_data_manager_backend.cc:404:
> > > std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end())) {
> > > > On 2016/05/17 03:32:16, Dan Beam wrote:
> > > > > On 2016/05/17 03:30:20, Avi wrote:
> > > > > > On 2016/05/17 03:24:14, Dan Beam wrote:
> > > > > > > |hosts| was never used because
> > > > > > > if (thing && (thing || other_thing))
> > > > > > > never hits other_thing.
> > > > > >
> > > > > > Right, but what CL introduced this silliness? What were they
> > intending
> > to
> > > do?
> > > > > > Were what they were worrying about something we should fix rather
> > than
> > > just
> > > > > rip
> > > > > > out?
> > > > >
> > > > > https://codereview.chromium.org/183803023/#msg10
> > > >
> > > > Yeah, this code is definitely broken after
> > > https://codereview.chromium.org/183803023/.
> > > >
> > > > That was a refactor to https://codereview.chromium.org/130963006,
> > which
> > moved
> > > chrome://chrome-signin from a webview to an iframe.
> > > >
> > > > Looks like the intent was to make sure that only whitelisted WebUI
> > hosts
> > could
> > > get past the StoragePartition check, but after the refactor *any*
> > chrome://
> > URL
> > > could (which meant powerful pages like chrome://settings, etc). That
> > kind of
> > > defeated the point of the check. (I'm not sure how we were able to have
> > an
> > > iframe with a different StoragePartition in the first place-- I don't
> > understand
> > > that part of the original CL's description.)
> > > >
> > > > However, if I understand correctly, we have since moved back to the
> > webview
> > > approach for sign-in, and the iframe approach is no longer used. And
> > yes, the
> > > old iframe based approach looks like it was removed in rogerta's
> > > https://codereview.chromium.org/1412453002.
> > > >
> > > > I'm not sure why this code wasn't removed along with the rest--
> > hopefully it
> > > isn't necessary to load any WebUI pages (within the host whitelist or
> > not)
> > > inside the webview, so this check shouldn't be needed. Maybe the entire
> > > CheckStoragePartitionMatches function can go away, since it was only
> > introduced
> > > for the iframe version? (Indeed, it seems a bit unsafe to leave it here,
> > > certainly as written. It looks like we block navigating the webview to
> > > chrome:// URLs in other ways when I try it out, but it feels risky to
> > have
> > this
> > > path around.)
> > > >
> > > > Roger, can you confirm whether this has any value now?
> > >
> > > Hi Charlie,
> > >
> > > The iframe based sign in mechanism is gone. I am not aware of this code
> > and
> > > maybe should have deleted it with my CL. I'm not working on chrome
> > sign-in
> > any
> > > more and this is all a while back. I don't remember why these exceptions
> > were
> > > needed. The best way to check if the code is still required: when
> > performing
> > a
> > > sign in to chrome:
> > >
> > > 1/ make sure the renderer process used to show the gaia sign in page is
> > not
> > > shared with any other existing renderer process
> > > 2/ make sure that the cookie jar of that gaia page is not shared with the
> > > profile's main cookie jar
> > > 3/ make sure sign in to chrome works
> >
> > Thanks. There's tests for these, right? Maybe dbeam can try removing this
> > code
> > entirely and seeing if the tests are happy?
> >
> > https://codereview.chromium.org/1985983002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

This code predates me by a lot and I can't find tests that (to my knowledge)
would fully cover this. The tests in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
should cover at least part of the signin process but my confidence in "them
passing => everything is fine" is fairly low since I don't know enough about
that part. There's also a couple scenarios we'd want to manually test.

In any case, we're definitely not using iframes for signin anymore and the only
possible navigations in the webview should be to SAML providers. From what I
understand, this is an exception to allow navigating to chrome:// pages, right?
That shouldn't happen in the signin flow so I don't think preventing this in
webviews would break signin.

[Avi (use Gerrit), 2016-05-18 21:42:30]

On 2016/05/18 20:47:46, anthonyvd wrote:
> This code predates me by a lot and I can't find tests that (to my knowledge)
> would fully cover this. The tests in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> should cover at least part of the signin process but my confidence in "them
> passing => everything is fine" is fairly low since I don't know enough about
> that part. There's also a couple scenarios we'd want to manually test.

Can you be more specific about what you would like Dan to test here? I'd like
you to be able to reasonably feel confident before giving this an OK.

[groby-ooo-7-16, 2016-05-18 22:05:28]

> From what I
> understand, this is an exception to allow navigating to chrome:// pages,
right?
> That shouldn't happen in the signin flow so I don't think preventing this in
> webviews would break signin.

It allows navigation to chrome:// pages within iframes, yes. This path is *only*
executed for chrome:// and chrome-devtools:// pages, and the storage partition
scenario only triggers in the latter scheme.

>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > should cover at least part of the signin process but my confidence in "them
> > passing => everything is fine" is fairly low since I don't know enough about
> > that part. There's also a couple scenarios we'd want to manually test.
> 
> Can you be more specific about what you would like Dan to test here? I'd like
> you to be able to reasonably feel confident before giving this an OK.

I'm all for reasonable test coverage, but let's keep in mind that this is a very
simple behavior-preserving change. If it's a chrome:// scheme, do a
StartAsync(true), otherwise check for storage partition match. The *only*
behavior change here is that we skip two unnecessary PostTasks for a chrome://
scheme URL. And the StoragePartition part isn't even covered by tests right now.

There is *nothing* sign-in specific in this code. It originally supported
sign-in, but only by not whitelisting it. 

I don't think this CL should become responsible for improving test coverage for
signin, FWIW.

[Charlie Reis, 2016-05-18 23:58:20]

On 2016/05/18 22:05:28, groby wrote:
> > From what I
> > understand, this is an exception to allow navigating to chrome:// pages,
> right?
> > That shouldn't happen in the signin flow so I don't think preventing this in
> > webviews would break signin.
> 
> It allows navigation to chrome:// pages within iframes, yes. This path is
*only*
> executed for chrome:// and chrome-devtools:// pages, and the storage partition
> scenario only triggers in the latter scheme.

Wait, how is chrome-devtools:// related to CheckStoragePartitionMatches?  It's
not depending on this check, is it?

> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > should cover at least part of the signin process but my confidence in
"them
> > > passing => everything is fine" is fairly low since I don't know enough
about
> > > that part. There's also a couple scenarios we'd want to manually test.
> > 
> > Can you be more specific about what you would like Dan to test here? I'd
like
> > you to be able to reasonably feel confident before giving this an OK.
> 
> I'm all for reasonable test coverage, but let's keep in mind that this is a
very
> simple behavior-preserving change. If it's a chrome:// scheme, do a
> StartAsync(true), otherwise check for storage partition match. The *only*
> behavior change here is that we skip two unnecessary PostTasks for a chrome://
> scheme URL. And the StoragePartition part isn't even covered by tests right
now.
> 
> There is *nothing* sign-in specific in this code. It originally supported
> sign-in, but only by not whitelisting it. 

The concern is that CheckStoragePartitionMatches is fundamentally broken in what
it was trying to accomplish at the time, as Dan discovered.  Refactoring it to
preserve that bug in a more efficient way doesn't really make sense, especially
since it was originally a security check.

Fortunately, I think we've almost concluded that the whole function can be
removed as dead code, which would be even better.


> I don't think this CL should become responsible for improving test coverage
for
> signin, FWIW.

We're not asking for this CL to introduce tests for sign-in.  We just want to
know how to tell if removing this broken code will cause a regression.

[groby-ooo-7-16, 2016-05-19 00:38:15]

> Wait, how is chrome-devtools:// related to CheckStoragePartitionMatches?  It's
> not depending on this check, is it?

Long convoluted story :) Essentially, this is only executed for
UrlRequestChromeJob. UrlRequestChromeJob is only executed for two protocol
handlers, chrome:// and chrome-devtools://
For the chrome:// case, we *always* take the shortcut path without
CheckStoragePartitionMatches, so this effectively only triggers on requests for
chrome-devtools://

It's "depending" on this check in the sense that if you have a renderer request
*any* chrome-devtools:// pages, that renderer must have a matching storage
partition.  Quoth the comment preceding the code in question: "We do an extra
check to guard against an exploited renderer pretending to add them as a
subframe. We skip this check for resources."

That's the extent of the dependency.

> The concern is that CheckStoragePartitionMatches is fundamentally broken in
what
> it was trying to accomplish at the time, as Dan discovered.  Refactoring it to
> preserve that bug in a more efficient way doesn't really make sense,
especially
> since it was originally a security check.

As I suggested to Dan offline - we can keep the intent of the original behavior
if we move the whitelist check into URLRequestChromeJob::Start as well. That's
the only broken part - it was supposed to check if the URL is a whitelisted
chrome:// resource URL, and to check the storage partition only for chrome://
URLs on the whitelist.

The "supposed to" behavior was originally implemented in
https://chromium.googlesource.com/chromium/src/+/4413d87abb4483aecd867cac1e07...
- specifically for chrome-signin. My seeming disregard for tests stems from the
fact chrome-signin doesn't use any chrome:// pages any more. So yes, we lost
behavior, but we also lost the use-case for that behavior. 

> Fortunately, I think we've almost concluded that the whole function can be
> removed as dead code, which would be even better.

I think you might be right there, which would make me very happy :)

> > I don't think this CL should become responsible for improving test coverage
> for
> > signin, FWIW.
> 
> We're not asking for this CL to introduce tests for sign-in.  

Apologies for misunderstanding.

> We just want to
> know how to tell if removing this broken code will cause a regression.

IIUC - and I'm by no means an URLRequest expert :) - completely removing this
code would lose us the storage partition check, which is a bad thing, according
to the comment above the check: "RenderProcessHostImpl::IsSuitableHost would
guard against top level pages being in the same process. We do an extra check to
guard against an exploited renderer pretending to add them as a subframe." Maybe
that's a total belt-and-suspenders approach that we don't need anymore. Maybe it
was only there for signing. I've dug back some more for the original CL
introducing the CheckStoragePartitionMatch behavior for the first time:
https://codereview.chromium.org/134263005

And reading the CL comment, yes, this got introduced _specifically_ for the
inline signin page. If we're not using that any more (and looking at
ChromeContentBrowserClient::GetStoragePartitionIdForSite, the custom
StoragePartitionId necessary for this is gone, so I assume the page is gone),
then yes, this can probably be killed. 

Since this only exists for a specific use case that was removed since, we can't
really figure out if there are regressions. There's nobody using this any more. 

Here's a suggestion to put us somewhat at ease:
* We fix CheckStoragePartitionMatches to have the intended original behavior:
Only whitelisted chrome:// parts are allowed to bypass the storage partition
check. 
* We stick a DCHECK into the actual partition comparison path.
* We then run all the trybots.

If nothing blows up, that means we have _no_ test cases exercising this right
now - which means worst case we're as badly off as the last two years - and we
have reasonable confidence from code inspection that indeed this isn't needed
anymore, since signin pages are no more.

Does that sound good?

[Dan Beam, 2016-05-19 03:57:43]

re-instates the host filtering in a more efficient way.  we'll have to figure
out a good spot to add the CHECK groby@'s talking about.

will issue try jobs and see what blows up.

https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:279:
hosts.push_back(content::kChromeUIResourcesHost);
fixes this issue: https://codereview.chromium.org/183803023/#msg11

[Dan Beam, 2016-05-19 06:46:26]

https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:295:
TRACE_EVENT_ASYNC_BEGIN1("browser", "DataManager:Request", this, "URL",
btw, I'll move this to the top of the method tomorrow

[Roger Tawa OOO till Jul 10th, 2016-05-19 12:01:56]

Wrt:

"""
My seeming disregard for tests stems from the
fact chrome-signin doesn't use any chrome:// pages any more.
"""

The top level sign in URL is chrome://chrome-signin. This top level page
now embeds a webview instead of an iframe. So not sure that statement is
completely correct.
On May 19, 2016 02:46, <dbeam@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
> File content/browser/webui/url_data_manager_backend.cc (right):
>
>
>
https://codereview.chromium.org/1985983002/diff/20001/content/browser/webui/u...
> content/browser/webui/url_data_manager_backend.cc:295:
> TRACE_EVENT_ASYNC_BEGIN1("browser", "DataManager:Request", this, "URL",
> btw, I'll move this to the top of the method tomorrow
>
> https://codereview.chromium.org/1985983002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2016-05-19 20:31:04]

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

[commit-bot: I haz the power, 2016-05-19 20:31:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1985983002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1985983002/40001

[Charlie Reis, 2016-05-19 20:35:50]

I'm happy with patch set 3.  It's a clear optimization, and it fixes a bug in
the old code.  In this state, the StoragePartition check works as originally
intended (for the defunct case of iframe-based signin).  It makes sure that
chrome:// pages (excluding the whitelist of hosts) and chrome-devtools:// pages
don't load subframes that belong in a different StoragePartition.

It's possible we could go further and remove the whole partition check, since
the iframe-based signin code that depended on it is now gone.  Since that
carries some additional risk, I'm ok with leaving it as a TODO in this CL.

Thanks for bearing with us, Dan!  LGTM.  Also, please remove the second
paragraph from the CL description and list 338127 as the bug number, to
associate this change with the iframe signin work.

https://codereview.chromium.org/1985983002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1985983002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:280: if
(url.SchemeIs(kChromeUIScheme)) {
Can we add a TODO here to remove the whole CheckStoragePartitionMatches check if
we can determine it's safe to do so (given that it was added for iframe-based
signin, which no longer exists)?

[commit-bot: I haz the power, 2016-05-19 20:43:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 20:43:17]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Dan Beam, 2016-05-19 23:45:59]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, remove GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

[Dan Beam, 2016-05-19 23:46:10]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck which just
seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

[Dan Beam, 2016-05-19 23:46:36]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=none
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127
==========

[Dan Beam, 2016-05-19 23:46:46]

and updated CL desc

https://codereview.chromium.org/1985983002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1985983002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:280: if
(url.SchemeIs(kChromeUIScheme)) {
On 2016/05/19 20:35:50, Charlie Reis (slow to reply) wrote:
> Can we add a TODO here to remove the whole CheckStoragePartitionMatches check
if
> we can determine it's safe to do so (given that it was added for iframe-based
> signin, which no longer exists)?

Done.

[Dan Beam, 2016-05-19 23:46:54]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-05-19 23:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1985983002/#ps60001
(title: "compile fix")

[commit-bot: I haz the power, 2016-05-19 23:47:53]

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

[commit-bot: I haz the power, 2016-05-20 01:29:05]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127
==========

[commit-bot: I haz the power, 2016-05-20 01:29:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 01:30:32]

Description was changed from

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127
==========

to

==========
WebUI: shuffle less between threads when responding to requests

Also, re-enable GetAdditionalWebUIHostsToIgnoreParititionCheck
which just seemed to be dead code for the last 2.2 years.

R=avi@chromium.org
BUG=338127

Committed: https://crrev.com/78b98db5654803262d752a07050e7d77f6cc6bfa
Cr-Commit-Position: refs/heads/master@{#394944}
==========

[commit-bot: I haz the power, 2016-05-20 01:30:34]

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