Don't show the pending URL for chrome.tabs API navigations.

Don't show the pending URL for chrome.tabs API navigations.

BUG=660498
TEST=See bug for repro steps.

[Closed in favor of r431726]

[Charlie Reis, 2016-11-03 17:39:58]

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

[commit-bot: I haz the power, 2016-11-03 17:40:30]

Dry run: 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-11-03 18:16:09]

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

[commit-bot: I haz the power, 2016-11-03 18:16:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-11-03 20:38:34]

creis@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Charlie Reis, 2016-11-03 20:38:35]

Devlin, can I get your thoughts on this?  In PS1, the change is only failing the
ExtensionApiTest.Tabs2 test, which seems to expect WebContents::GetURL() to
return the URL we're navigating to, before it commits.  PS2 identifies the part
of the test that's failing, but it's not fixed or ready to land yet.

This test behavior is concerning in a few ways:

1) WebContents::GetURL() is deprecated and hard to use correctly, because the
visible URL can change for a bunch of reasons (related to when navigations
start, commit, or are hidden due to spoof-like behavior on the page).  It's
surprising to me that we would expose the visible URL in the tabs API rather
than the last committed URL (or both), since those have importantly different
meanings.

2) I think there's almost no visible change to users when making chrome.tabs be
treated as a renderer-initiated navigation (since we don't refresh the omnibox
automatically anyway), but it looks like it could be a change visible to
extensions based on this test.  Unclear if it would break anything.

3) There doesn't seem to be a way to check what the pending URL is in the test,
and I'm not sure how to make it wait for commit.  If we do continue in this
direction, we'll need to find a way to get the test to work.

I suppose we could somehow look for a way to make this change only apply to the
Mime Handler extension rather than all extensions if needed.  What do you think?

[Devlin, 2016-11-04 18:03:48]

Description was changed from

==========
Don't show the pending URL for chrome.tabs API navigations.

BUG=660498
TEST=See bug for repro steps.
==========

to

==========
Don't show the pending URL for chrome.tabs API navigations.

BUG=660498
TEST=See bug for repro steps.
==========

[Devlin, 2016-11-04 18:03:48]

rdevlin.cronin@chromium.org changed reviewers:
+ nasko@chromium.org

[Devlin, 2016-11-04 18:04:06]

rdevlin.cronin@chromium.org changed reviewers:
- nasko@chromium.org

[Devlin, 2016-11-04 18:12:34]

On 2016/11/03 20:38:35, Charlie Reis wrote:
> Devlin, can I get your thoughts on this?  In PS1, the change is only failing
the
> ExtensionApiTest.Tabs2 test, which seems to expect WebContents::GetURL() to
> return the URL we're navigating to, before it commits.  PS2 identifies the
part
> of the test that's failing, but it's not fixed or ready to land yet.
> 
> This test behavior is concerning in a few ways:
> 
> 1) WebContents::GetURL() is deprecated and hard to use correctly, because the
> visible URL can change for a bunch of reasons (related to when navigations
> start, commit, or are hidden due to spoof-like behavior on the page).  It's
> surprising to me that we would expose the visible URL in the tabs API rather
> than the last committed URL (or both), since those have importantly different
> meanings.
> 
> 2) I think there's almost no visible change to users when making chrome.tabs
be
> treated as a renderer-initiated navigation (since we don't refresh the omnibox
> automatically anyway), but it looks like it could be a change visible to
> extensions based on this test.  Unclear if it would break anything.
> 
> 3) There doesn't seem to be a way to check what the pending URL is in the
test,
> and I'm not sure how to make it wait for commit.  If we do continue in this
> direction, we'll need to find a way to get the test to work.
> 
> I suppose we could somehow look for a way to make this change only apply to
the
> Mime Handler extension rather than all extensions if needed.  What do you
think?

Nasko (cc'd) and I were just chatting about this the other day, in the context
of updating the way we create tabs objects to use the last committed instead of
visual url
(https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_tab_...).
 The conclusion was that in almost all cases, that would be fine (and probably
preferable), but it would lead to weirdness in the case that you found in the
test:
if an extension does
chrome.tabs.update(id, {url: example.com}, function(tab) { ... });
it would be expected that the tab object have the example.com url (and I
wouldn't be surprised if some extensions used this as a measure to check
success, even though they probably shouldn't).

Given this is coming up in more than one place, I'm more and more tempted to
change it, and wonder if there's a way we can make it reasonable.  One step to
this would be communicating the change to developers and trying to let this roll
through normal release channels (rather than merge).  Additionally, I think
adding a pendingUrl property on the tab object is something that's fairly
reasonable, and would make this distinction a little more clear (and in the
test, we would just check that the pendingUrl == foo).

Any thoughts on the feasibility of that?  And is this something we'll need to
merge back (or merge back an alternate fix)?

[Charlie Reis, 2016-11-04 18:28:58]

On 2016/11/04 18:12:34, Devlin (slow) wrote:
> On 2016/11/03 20:38:35, Charlie Reis wrote:
> > Devlin, can I get your thoughts on this?  In PS1, the change is only failing
> the
> > ExtensionApiTest.Tabs2 test, which seems to expect WebContents::GetURL() to
> > return the URL we're navigating to, before it commits.  PS2 identifies the
> part
> > of the test that's failing, but it's not fixed or ready to land yet.
> > 
> > This test behavior is concerning in a few ways:
> > 
> > 1) WebContents::GetURL() is deprecated and hard to use correctly, because
the
> > visible URL can change for a bunch of reasons (related to when navigations
> > start, commit, or are hidden due to spoof-like behavior on the page).  It's
> > surprising to me that we would expose the visible URL in the tabs API rather
> > than the last committed URL (or both), since those have importantly
different
> > meanings.
> > 
> > 2) I think there's almost no visible change to users when making chrome.tabs
> be
> > treated as a renderer-initiated navigation (since we don't refresh the
omnibox
> > automatically anyway), but it looks like it could be a change visible to
> > extensions based on this test.  Unclear if it would break anything.
> > 
> > 3) There doesn't seem to be a way to check what the pending URL is in the
> test,
> > and I'm not sure how to make it wait for commit.  If we do continue in this
> > direction, we'll need to find a way to get the test to work.
> > 
> > I suppose we could somehow look for a way to make this change only apply to
> the
> > Mime Handler extension rather than all extensions if needed.  What do you
> think?
> 
> Nasko (cc'd) and I were just chatting about this the other day, in the context
> of updating the way we create tabs objects to use the last committed instead
of
> visual url
>
(https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_tab_...).
>  The conclusion was that in almost all cases, that would be fine (and probably
> preferable), but it would lead to weirdness in the case that you found in the
> test:
> if an extension does
> chrome.tabs.update(id, {url: example.com}, function(tab) { ... });
> it would be expected that the tab object have the http://example.com url (and
I
> wouldn't be surprised if some extensions used this as a measure to check
> success, even though they probably shouldn't).
> 
> Given this is coming up in more than one place, I'm more and more tempted to
> change it, and wonder if there's a way we can make it reasonable.  One step to
> this would be communicating the change to developers and trying to let this
roll
> through normal release channels (rather than merge).  Additionally, I think
> adding a pendingUrl property on the tab object is something that's fairly
> reasonable, and would make this distinction a little more clear (and in the
> test, we would just check that the pendingUrl == foo).
> 
> Any thoughts on the feasibility of that?  And is this something we'll need to
> merge back (or merge back an alternate fix)?

For this bug, a merge to M55 would be nice to have but likely not critical
(since it only matters if the user switches tabs and comes back).  Maybe letting
it bake is ok, in the hopes of making things more sane overall.  Alternatively,
is it remotely possible to make a change like this just for the Mime Handler
extension and not others, in case we need something to land quickly and merge?

We'll need to be careful about the pendingUrl idea, though, because there are
several URL concepts with very different implications:
1) Last committed URL.  This is what's showing within the tab itself, and it
matters if you care about what page the user is interacting with.  The one
exception is when an interstitial overlay page is showing, in which case this is
what's under the interstitial (at least, until we refactor interstitials from
overlays into error pages).
2) Pending URL.  This is what navigation is currently in progress.  It is often
not shown in the address bar, due to concerns about URL spoofs.
3) Visible URL.  This generally matches what the omnibox is showing (with the
caveat that the omnibox may need to be manually refreshed to agree, as we've
seen here).  It's complicated-- it'll be the interstitial URL if there is one,
the pending URL if we think it's safe (for a complex set of reasons), or the
last committed URL otherwise.

The API is currently revealing the visible URL, which is probably hard for
people to reason about unless they actually care what the omnibox would show. 
Seems like we agree it may not the best option.

I'd hate to expose all three in the API.  It's hard to say which one(s) are best
to expose without knowing more about how the tab.url property is used in
practice, though.

[Devlin, 2016-11-04 20:35:35]

On 2016/11/04 18:28:58, Charlie Reis wrote:
> For this bug, a merge to M55 would be nice to have but likely not critical
> (since it only matters if the user switches tabs and comes back).  Maybe
letting
> it bake is ok, in the hopes of making things more sane overall. 
Alternatively,
> is it remotely possible to make a change like this just for the Mime Handler
> extension and not others, in case we need something to land quickly and merge?

Having separate behavior for a specific extension is doable, but hacky.  I would
be fine with it for a milestone or so (mostly because we're changing behavior in
theory for the better), but would want a definite plan for its removal.  As a
way to address the bug without breaking everything though, I'd be okay with it.

> We'll need to be careful about the pendingUrl idea, though, because there are
> several URL concepts with very different implications:
> 1) Last committed URL.  This is what's showing within the tab itself, and it
> matters if you care about what page the user is interacting with.  The one
> exception is when an interstitial overlay page is showing, in which case this
is
> what's under the interstitial (at least, until we refactor interstitials from
> overlays into error pages).
> 2) Pending URL.  This is what navigation is currently in progress.  It is
often
> not shown in the address bar, due to concerns about URL spoofs.
> 3) Visible URL.  This generally matches what the omnibox is showing (with the
> caveat that the omnibox may need to be manually refreshed to agree, as we've
> seen here).  It's complicated-- it'll be the interstitial URL if there is one,
> the pending URL if we think it's safe (for a complex set of reasons), or the
> last committed URL otherwise.
> 
> The API is currently revealing the visible URL, which is probably hard for
> people to reason about unless they actually care what the omnibox would show. 
> Seems like we agree it may not the best option.
> 
> I'd hate to expose all three in the API.  It's hard to say which one(s) are
best
> to expose without knowing more about how the tab.url property is used in
> practice, though.

Right, and all of its very interesting from an extension perspective.  From one
point of view, we're retrieving the "tab", which is a chrome browser concept and
distinct from the  "web page".  In this sense, returning the visible url could
actually make sense.  But this is almost certainly not what extensions are
actually after.

My suggestion would be that we use last committed for tab.url and use the
pending url for a pendingUrl property, which would mostly just be to allow
extensions to ensure navigation.  Although towards that point, they could also
just be using the webNavigation API... and again, this would be something I
wouldn't want to merge and would want to give some heads up about.

Unfortunately, with all of this its very difficult to know what this will break.
 Even if we could get metrics on how often the url property is used (which in
and of itself is tricky and probably not worth it for the complexity it would
introduce), we'd have no way of knowing how it was used.

[nasko, 2016-11-04 22:56:52]

On 2016/11/04 20:35:35, Devlin (slow) wrote:
> On 2016/11/04 18:28:58, Charlie Reis wrote:
> > For this bug, a merge to M55 would be nice to have but likely not critical
> > (since it only matters if the user switches tabs and comes back).  Maybe
> letting
> > it bake is ok, in the hopes of making things more sane overall. 
> Alternatively,
> > is it remotely possible to make a change like this just for the Mime Handler
> > extension and not others, in case we need something to land quickly and
merge?
> 
> Having separate behavior for a specific extension is doable, but hacky.  I
would
> be fine with it for a milestone or so (mostly because we're changing behavior
in
> theory for the better), but would want a definite plan for its removal.  As a
> way to address the bug without breaking everything though, I'd be okay with
it.
> 
> > We'll need to be careful about the pendingUrl idea, though, because there
are
> > several URL concepts with very different implications:
> > 1) Last committed URL.  This is what's showing within the tab itself, and it
> > matters if you care about what page the user is interacting with.  The one
> > exception is when an interstitial overlay page is showing, in which case
this
> is
> > what's under the interstitial (at least, until we refactor interstitials
from
> > overlays into error pages).
> > 2) Pending URL.  This is what navigation is currently in progress.  It is
> often
> > not shown in the address bar, due to concerns about URL spoofs.
> > 3) Visible URL.  This generally matches what the omnibox is showing (with
the
> > caveat that the omnibox may need to be manually refreshed to agree, as we've
> > seen here).  It's complicated-- it'll be the interstitial URL if there is
one,
> > the pending URL if we think it's safe (for a complex set of reasons), or the
> > last committed URL otherwise.
> > 
> > The API is currently revealing the visible URL, which is probably hard for
> > people to reason about unless they actually care what the omnibox would
show. 
> > Seems like we agree it may not the best option.
> > 
> > I'd hate to expose all three in the API.  It's hard to say which one(s) are
> best
> > to expose without knowing more about how the tab.url property is used in
> > practice, though.
> 
> Right, and all of its very interesting from an extension perspective.  From
one
> point of view, we're retrieving the "tab", which is a chrome browser concept
and
> distinct from the  "web page".  In this sense, returning the visible url could
> actually make sense.  But this is almost certainly not what extensions are
> actually after.
> 
> My suggestion would be that we use last committed for tab.url and use the
> pending url for a pendingUrl property, which would mostly just be to allow
> extensions to ensure navigation.  Although towards that point, they could also
> just be using the webNavigation API... and again, this would be something I
> wouldn't want to merge and would want to give some heads up about.

I personally would love to not expose anything but the last committed URL. I
have a CL where I tried to move over all tab generation to be
GetLastCommittedURL based and only keep the behavior for chrome.tabs.update()
callback to return the GetVisibleURL(). I know there are still some outstanding
work on it, but would that be a good compromise? This way extensions that want
to ensure they started a navigation to the intended URL can get the data
immediately in the callback to chrome.tabs.update(), but all other cases will
use the right one.

> Unfortunately, with all of this its very difficult to know what this will
break.
>  Even if we could get metrics on how often the url property is used (which in
> and of itself is tricky and probably not worth it for the complexity it would
> introduce), we'd have no way of knowing how it was used.

Yes, I don't think this is something that will be easily backportable,
unfortunately.

[Charlie Reis, 2016-11-07 23:03:22]

creis@chromium.org changed reviewers:
+ lfg@chromium.org, nasko@chromium.org

[Charlie Reis, 2016-11-07 23:03:23]

Ok.  Sounds like this is a little tricky, but the easiest way forward might be
pursuing Nasko's https://codereview.chromium.org/2434063002/ (to usually expose
the last committed URL but use the pending URL in the update callback), which
would fix the one failing test in this CL.  Note that his CL would have to
change from using the visible URL to the pending URL in the callback for that to
work.

With that approach, we wouldn't need to do any extension-specific work for the
PDF extension.  However, it also would leave extensions without much of a way to
query the pending URL until we later added something.  Thoughts?

(Note that I'm about to be OOO, so Nasko has offered to help make progress the
next few days.)

[Devlin, 2016-11-07 23:13:47]

On 2016/11/07 23:03:23, Charlie Reis (OOO til 11-11) wrote:
> Ok.  Sounds like this is a little tricky, but the easiest way forward might be
> pursuing Nasko's https://codereview.chromium.org/2434063002/ (to usually
expose
> the last committed URL but use the pending URL in the update callback), which
> would fix the one failing test in this CL.  Note that his CL would have to
> change from using the visible URL to the pending URL in the callback for that
to
> work.
> 
> With that approach, we wouldn't need to do any extension-specific work for the
> PDF extension.  However, it also would leave extensions without much of a way
to
> query the pending URL until we later added something.  Thoughts?
> 
> (Note that I'm about to be OOO, so Nasko has offered to help make progress the
> next few days.)

I'm not real keen on that because of the discontinuity it creates in terms of
the tabs objects (e.g. the tab object in onUpdated is different than the one in
the update callback, we get into hairiness with whether or not we want that
behavior for tabs.create(), etc) - especially when we don't necessarily know the
most common ways developers are relying on this.  I think my preference would to
be make a single change to make everything correct rather than a series of
smaller breaking changes that we think (but don't really know) are less severe. 
Unfortunately, I'm not sure I'd be comfortable with either one of those being
rushed (and I understand that this has decent priority).

How would y'all feel about doing the fix just for the single extension while we
sort out the rest of this to try and end up in the best state possible?

[Charlie Reis, 2016-11-08 01:44:02]

On 2016/11/07 23:13:47, Devlin (slow) wrote:
> On 2016/11/07 23:03:23, Charlie Reis (OOO til 11-11) wrote:
> > Ok.  Sounds like this is a little tricky, but the easiest way forward might
be
> > pursuing Nasko's https://codereview.chromium.org/2434063002/ (to usually
> expose
> > the last committed URL but use the pending URL in the update callback),
which
> > would fix the one failing test in this CL.  Note that his CL would have to
> > change from using the visible URL to the pending URL in the callback for
that
> to
> > work.
> > 
> > With that approach, we wouldn't need to do any extension-specific work for
the
> > PDF extension.  However, it also would leave extensions without much of a
way
> to
> > query the pending URL until we later added something.  Thoughts?
> > 
> > (Note that I'm about to be OOO, so Nasko has offered to help make progress
the
> > next few days.)
> 
> I'm not real keen on that because of the discontinuity it creates in terms of
> the tabs objects (e.g. the tab object in onUpdated is different than the one
in
> the update callback, we get into hairiness with whether or not we want that
> behavior for tabs.create(), etc) - especially when we don't necessarily know
the
> most common ways developers are relying on this.  I think my preference would
to
> be make a single change to make everything correct rather than a series of
> smaller breaking changes that we think (but don't really know) are less
severe. 
> Unfortunately, I'm not sure I'd be comfortable with either one of those being
> rushed (and I understand that this has decent priority).
> 
> How would y'all feel about doing the fix just for the single extension while
we
> sort out the rest of this to try and end up in the best state possible?

If there's a way to do that for just the PDF extension, I'm ok with that for the
short term.  I'll have to let someone else write it up, though.

[nasko, 2016-11-16 04:53:03]

On 2016/11/08 01:44:02, Charlie Reis (slow) wrote:
> On 2016/11/07 23:13:47, Devlin (slow) wrote:
> > On 2016/11/07 23:03:23, Charlie Reis (OOO til 11-11) wrote:
> > > Ok.  Sounds like this is a little tricky, but the easiest way forward
might
> be
> > > pursuing Nasko's https://codereview.chromium.org/2434063002/ (to usually
> > expose
> > > the last committed URL but use the pending URL in the update callback),
> which
> > > would fix the one failing test in this CL.  Note that his CL would have to
> > > change from using the visible URL to the pending URL in the callback for
> that
> > to
> > > work.
> > > 
> > > With that approach, we wouldn't need to do any extension-specific work for
> the
> > > PDF extension.  However, it also would leave extensions without much of a
> way
> > to
> > > query the pending URL until we later added something.  Thoughts?
> > > 
> > > (Note that I'm about to be OOO, so Nasko has offered to help make progress
> the
> > > next few days.)
> > 
> > I'm not real keen on that because of the discontinuity it creates in terms
of
> > the tabs objects (e.g. the tab object in onUpdated is different than the one
> in
> > the update callback, we get into hairiness with whether or not we want that
> > behavior for tabs.create(), etc) - especially when we don't necessarily know
> the
> > most common ways developers are relying on this.  I think my preference
would
> to
> > be make a single change to make everything correct rather than a series of
> > smaller breaking changes that we think (but don't really know) are less
> severe. 
> > Unfortunately, I'm not sure I'd be comfortable with either one of those
being
> > rushed (and I understand that this has decent priority).
> > 
> > How would y'all feel about doing the fix just for the single extension while
> we
> > sort out the rest of this to try and end up in the best state possible?
> 
> If there's a way to do that for just the PDF extension, I'm ok with that for
the
> short term.  I'll have to let someone else write it up, though.

Since we already landed a fix for this as part of another CL, should this one be
closed?

[Charlie Reis, 2016-11-16 17:35:41]

Description was changed from

==========
Don't show the pending URL for chrome.tabs API navigations.

BUG=660498
TEST=See bug for repro steps.
==========

to

==========
Don't show the pending URL for chrome.tabs API navigations.

BUG=660498
TEST=See bug for repro steps.

[Closed in favor of r431726]
==========

[Charlie Reis, 2016-11-16 17:35:58]

On 2016/11/16 04:53:03, nasko (out until Nov 17) wrote:
> Since we already landed a fix for this as part of another CL, should this one
be
> closed?

Done, thanks.