Invalidate the page state when restoring WebUIs

Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720
Cr-Commit-Position: refs/heads/master@{#420600}

[calamity, 2016-09-20 07:32:07]

Patchset #1 (id:1) has been deleted

[calamity, 2016-09-21 04:59:06]

Description was changed from

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized  page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
==========

to

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
==========

[calamity, 2016-09-21 05:06:51]

calamity@chromium.org changed reviewers:
+ sky@chromium.org

[calamity, 2016-09-21 05:06:52]

PTAL, thanks!

[Dan Beam, 2016-09-21 06:19:17]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-09-21 06:19:17]

lgtm

https://codereview.chromium.org/2355543003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/2355543003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:198: bool MdHistoryUI::use_test_title =
false;
should this technically be g_use_test_title_?

https://codereview.chromium.org/2355543003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:212:
CreateMdHistoryUIHTMLSource(profile, MdHistoryUI::use_test_title));
I don't technically think you need this MdHistoryUI prefix

[calamity, 2016-09-21 06:37:15]

Description was changed from

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
==========

to

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-09-21 06:37:20]

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

[commit-bot: I haz the power, 2016-09-21 06:37:43]

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

[calamity, 2016-09-21 06:40:34]

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

[commit-bot: I haz the power, 2016-09-21 06:40:38]

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

[calamity, 2016-09-21 06:47:14]

Patchset #3 (id:60001) has been deleted

[calamity, 2016-09-21 07:06:43]

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

[commit-bot: I haz the power, 2016-09-21 07:07:07]

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-09-21 07:18:09]

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

[commit-bot: I haz the power, 2016-09-21 07:18:10]

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_...)

[calamity, 2016-09-21 07:30:18]

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

[commit-bot: I haz the power, 2016-09-21 07:30:32]

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-09-21 07:33:33]

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

[commit-bot: I haz the power, 2016-09-21 07:33:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[groby-ooo-7-16, 2016-09-21 14:36:12]

On 2016/09/21 05:06:52, calamity wrote:
> PTAL, thanks!

Thanks for digging into this!

You might want to ask creis to take a quick glance - he's probably the most
familiar with all the wonders of serialized page state.

[sky, 2016-09-21 16:11:17]

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:489:
IN_PROC_BROWSER_TEST_F(ContinueWhereILeftOffTest, MDHistoryUpgrade) {
Do you really need a browser test for this? Can't you write a unit test that
that verifies this?

https://codereview.chromium.org/2355543003/diff/120001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/120001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:106: //
Clear any WebUI page state.
This just documents what the code does. The interesting part is why this is
necessary. Please document the why.

[calamity, 2016-09-22 01:42:22]

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

[commit-bot: I haz the power, 2016-09-22 01:42:46]

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

[calamity, 2016-09-22 01:43:24]

calamity@chromium.org changed reviewers:
+ creis@chromium.org

[calamity, 2016-09-22 01:43:25]

+creis for the Wonders of Page State.

Does this seem kosher to you?

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:489:
IN_PROC_BROWSER_TEST_F(ContinueWhereILeftOffTest, MDHistoryUpgrade) {
On 2016/09/21 16:11:16, sky wrote:
> Do you really need a browser test for this? Can't you write a unit test that
> that verifies this?

I can verify that the page state is reset for WebUIs in a unit test, but I
really want to verify that session restore works correctly when the URL
switches. Session restore and page state stuff is pretty complex so I'm not sure
how I'd test this in a unit test.

https://codereview.chromium.org/2355543003/diff/120001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/120001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:106: //
Clear any WebUI page state.
On 2016/09/21 16:11:16, sky wrote:
> This just documents what the code does. The interesting part is why this is
> necessary. Please document the why.

Done.

[commit-bot: I haz the power, 2016-09-22 02:24:45]

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

[commit-bot: I haz the power, 2016-09-22 02:24:46]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-22 13:21:37]

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:489:
IN_PROC_BROWSER_TEST_F(ContinueWhereILeftOffTest, MDHistoryUpgrade) {
On 2016/09/22 01:43:25, calamity wrote:
> On 2016/09/21 16:11:16, sky wrote:
> > Do you really need a browser test for this? Can't you write a unit test that
> > that verifies this?
> 
> I can verify that the page state is reset for WebUIs in a unit test, but I
> really want to verify that session restore works correctly when the URL
> switches. Session restore and page state stuff is pretty complex so I'm not
sure
> how I'd test this in a unit test.

Verifying session restore is triggered on a title change seems separate from
your patch. I agree that is worth checking, but isn't really part of this patch.

[Dan Beam, 2016-09-22 18:09:20]

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/120001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:489:
IN_PROC_BROWSER_TEST_F(ContinueWhereILeftOffTest, MDHistoryUpgrade) {
On 2016/09/22 13:21:37, sky wrote:
> On 2016/09/22 01:43:25, calamity wrote:
> > On 2016/09/21 16:11:16, sky wrote:
> > > Do you really need a browser test for this? Can't you write a unit test
that
> > > that verifies this?
> > 
> > I can verify that the page state is reset for WebUIs in a unit test, but I
> > really want to verify that session restore works correctly when the URL
> > switches. Session restore and page state stuff is pretty complex so I'm not
> sure
> > how I'd test this in a unit test.
> 
> Verifying session restore is triggered on a title change seems separate from
> your patch. I agree that is worth checking, but isn't really part of this
patch.

hey Scott,

I think Chris is just checking that the correct page loaded.  Both new and old
version of the History UI live on chrome://history and have the title "History".
 Chris added a global flag that changes the new UI's title to "MD History" so we
can tell the difference.

I believe it's all just to tell that the page actually loaded.

Right now (before this patch), when you have a chrome://history page open and
restart Chrome (and trigger session restore), the chrome://history tab is just
broken (like an Error page).

-- Dan

[Charlie Reis, 2016-09-22 18:59:52]

I'm concerned this will degrade all history navigations to all WebUI pages. 
Maybe we can find a way to narrow the impact.

https://codereview.chromium.org/2355543003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/140001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:507: }
Please also test going back from enabled (no iframes) to disabled (with
iframes), since that's the one I saw breaking in practice.

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:110:
navigation->original_request_url_.SchemeIs(content::kChromeUIScheme)) {
This seems like it affects too many cases.  For URLs like chrome://about,
chrome://accessibility, chrome://net-internals, etc, we'll lose any scroll
position and form state if we navigate away and come back.  That's a functional
regression that we should avoid.

Is there any way to limit this to the URLs affected, and/or to the cases when
the state is out of date (perhaps by peeking into the PageState)?

[Dan Beam, 2016-09-22 19:37:34]

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:110:
navigation->original_request_url_.SchemeIs(content::kChromeUIScheme)) {
On 2016/09/22 18:59:51, Charlie Reis (slow) wrote:
> This seems like it affects too many cases.  For URLs like chrome://about,
> chrome://accessibility, chrome://net-internals, etc, we'll lose any scroll
> position and form state if we navigate away and come back.  That's a
functional
> regression that we should avoid.
> 
> Is there any way to limit this to the URLs affected, and/or to the cases when
> the state is out of date (perhaps by peeking into the PageState)?

do those pages set different virtual URLs?  could we check if the origins of the
virtual and original request URL differ?

[sky, 2016-09-22 20:33:19]

Ok, LGTM

[calamity, 2016-09-23 05:06:34]

https://codereview.chromium.org/2355543003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/2355543003/diff/140001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:507: }
On 2016/09/22 18:59:51, Charlie Reis (slow) wrote:
> Please also test going back from enabled (no iframes) to disabled (with
> iframes), since that's the one I saw breaking in practice.

Ah, you've landed the other patch I see. Great!

Done.

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:110:
navigation->original_request_url_.SchemeIs(content::kChromeUIScheme)) {
On 2016/09/22 18:59:51, Charlie Reis (slow) wrote:
> This seems like it affects too many cases.  For URLs like chrome://about,
> chrome://accessibility, chrome://net-internals, etc, we'll lose any scroll
> position and form state if we navigate away and come back.  That's a
functional
> regression that we should avoid.
> 
> Is there any way to limit this to the URLs affected, and/or to the cases when
> the state is out of date (perhaps by peeking into the PageState)?

I don't think we can figure out if the page state is out of date.. At least, not
without reintroducing all our profile problems.

How about limiting this to just MD History and Uber (and MD Settings?) URLs?
They already seem to have no proper state restore.

[Charlie Reis, 2016-09-23 06:33:53]

LGTM if you're ok with being stuck unable to restore PageState on those URLs
going forward.

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/140001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:110:
navigation->original_request_url_.SchemeIs(content::kChromeUIScheme)) {
On 2016/09/23 05:06:33, calamity wrote:
> On 2016/09/22 18:59:51, Charlie Reis (slow) wrote:
> > This seems like it affects too many cases.  For URLs like chrome://about,
> > chrome://accessibility, chrome://net-internals, etc, we'll lose any scroll
> > position and form state if we navigate away and come back.  That's a
> functional
> > regression that we should avoid.
> > 
> > Is there any way to limit this to the URLs affected, and/or to the cases
when
> > the state is out of date (perhaps by peeking into the PageState)?
> 
> I don't think we can figure out if the page state is out of date.. At least,
not
> without reintroducing all our profile problems.
> 
> How about limiting this to just MD History and Uber (and MD Settings?) URLs?
> They already seem to have no proper state restore.

Confirmed that chrome://settings and chrome://history don't seem to preserve any
PageState today (at least, scroll position or form data).

Of course, that seems like a bug to me, and this would prevent us from ever
fixing it (since we have to be able to restore old sessions).  That seems
unfortunate, but I suppose I'm ok with it if you are.  Maybe some day we can
find a more precise way to clear this and then fix the missing PageState problem
for those pages.

https://codereview.chromium.org/2355543003/diff/160001/components/sessions/co...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2355543003/diff/160001/components/sessions/co...
components/sessions/content/content_serialized_navigation_driver.cc:113: //
Clear any WebUI page state so that WebUI pages reloaded rather than
nit: are reloaded
nit: Update stale references to all WebUI pages.

[calamity, 2016-09-23 07:03:47]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-09-23 07:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
sky@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2355543003/#ps180001
(title: "fix nit")

[commit-bot: I haz the power, 2016-09-23 07:04:01]

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-09-23 11:37:48]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-09-23 11:40:47]

Description was changed from

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG=614758
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720
Cr-Commit-Position: refs/heads/master@{#420600}
==========

[commit-bot: I haz the power, 2016-09-23 11:40:48]

Patchset 8 (id:??) landed as
https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720
Cr-Commit-Position: refs/heads/master@{#420600}

[Dan Beam, 2017-03-14 05:43:48]

https://codereview.chromium.org/2355543003/diff/180001/content/public/common/...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2355543003/diff/180001/content/public/common/...
content/public/common/url_constants.h:36: CONTENT_EXPORT extern const char
kChromeUIHistoryHost[];
why do we need this in both chrome/common/url_constants.h AND
content/public/common/url_constants.h?