Avoid mutating frame unique name after first real commit.

Avoid mutating frame unique name after first real commit.

History navigations depend on frame unique names to identify frames that
need to load their state from history.  This identification is broken if
a frame's unique name can change throughout frame's lifetime.  For
example - the newly added layout tests fails (before FrameTree.cpp
changes introduced by this CL) to correctly navigate the subframe after
a back navigation.

BUG=607205
Committed: https://crrev.com/2ae993fad864e5c683d009ebdfbb0bdae3faef44
Cr-Commit-Position: refs/heads/master@{#418350}

[Łukasz Anforowicz, 2016-09-07 17:58:11]

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

[commit-bot: I haz the power, 2016-09-07 17:58:36]

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

[Łukasz Anforowicz, 2016-09-07 18:01:25]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2016-09-07 18:01:26]

Daniel, can you take a look please?  This implements the change we've discussed
between you, Charlie and me around 2 weeks ago.  Back then we were thinking
about first adding UMA to see how often window.name assignment happens
before-vs-after the first real commit, but now I think this is not necessary,
because
1. This CL doesn't change behavior for setting window.name *before* the first
real commit
2. Before this CL, the history behavior was broken if window.name was set
*after* the first real commit.  Therefore this CL is not a breaking change,
because there is nothing to break.

[commit-bot: I haz the power, 2016-09-07 18:42:45]

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

[commit-bot: I haz the power, 2016-09-07 18:42:46]

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

[dcheng, 2016-09-07 19:54:14]

Also it looks like there are some broken tests.

https://codereview.chromium.org/2317203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2317203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:78: if
(m_thisFrame->isLocalFrame()) {
This check is redundant (see the DCHECK on line 59 =)

[Łukasz Anforowicz, 2016-09-07 20:44:39]

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

[commit-bot: I haz the power, 2016-09-07 20:45:35]

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

[Łukasz Anforowicz, 2016-09-07 21:55:27]

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

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

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-07 23:08:13]

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

[commit-bot: I haz the power, 2016-09-07 23:08:14]

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

[Łukasz Anforowicz, 2016-09-07 23:29:38]

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

[commit-bot: I haz the power, 2016-09-07 23:30:47]

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-07 23:34:45]

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

[commit-bot: I haz the power, 2016-09-07 23:34:46]

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

[Łukasz Anforowicz, 2016-09-09 16:21:39]

On 2016/09/07 19:54:14, dcheng wrote:
> Also it looks like there are some broken tests.

Yes, my bad :-(  I've made additional changes to make the tests pass:
- Relaxed DCHECKs that were verifying that name is kept in-sync with unique-name
- Updated test expectations in 4 tests to look for the first unique name.

I also stopped updating / calculating unique name for the main frame.  As we
discussed in email, nobody is looking today at unique name of the main frame
(not session history + not layout tests).  Keeping the unique name of the main
frame set to null should also be okay from uniqueness perspective (i.e. it
should not interfere/conflict with unique names calculated for subframes,
because these unique names can never be null).

Can you take another look please?

https://codereview.chromium.org/2317203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2317203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:78: if
(m_thisFrame->isLocalFrame()) {
On 2016/09/07 19:54:13, dcheng wrote:
> This check is redundant (see the DCHECK on line 59 =)

Done.

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (left):

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:96: DCHECK(uniqueName ==
name);
We can no longer have the DCHECK above, because after this CL name and
unique-name are no longer kept in-sync (because m_uniqueName is immutable, but
m_name can keep changing).

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:65: || (m_uniqueName.isNull()
&& !parent()));
m_name and m_uniqueName are no longer in-sync now (because m_uniqueName is
immutable, but m_name can keep changing).  Therefore we need to relax the DCHECK
above.

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:83: return;
I was wondering whether it might be a good idea to (in DCHECK_IS_ON builds?)
assign random garbage to m_uniqueName in case of a main frame (to double-check
that nobody looks at the unique name of a main frame).

[dcheng, 2016-09-09 22:00:13]

LGTM

I have two concerns here:

1) this is going to make it impossible for ads that abuse long window.names to
mitigate their impact. So we'll have to figure out what we want to do there Real
Soon Now (tm). Charlie, if you have some time, let's chat real quickly and see
if we're comfortable moving ahead with the hammer and throwing out backwards
compat.

2) subframe names that collided with the main frame's name will now have
different unique names. This is probably OK... but something to be aware of. I
guess we don't have any measurements of how common that is?

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html
(right):

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html:14:
if (sessionStorage.alreadyRenamedSubframeAndTrigerredNavigations)
Nit:triggered

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:83: return;
On 2016/09/09 16:21:38, Łukasz Anforowicz wrote:
> I was wondering whether it might be a good idea to (in DCHECK_IS_ON builds?)
> assign random garbage to m_uniqueName in case of a main frame (to double-check
> that nobody looks at the unique name of a main frame).

I think this is fine (and if they're using it, that's too bad)

[Charlie Reis, 2016-09-09 22:11:33]

Can you also add a bit of motivation to the CL description, covering both the
subframe back/forward problem and the cleaner invariant we get with always
having the main frame's unique name be empty?

On 2016/09/09 22:00:13, dcheng wrote:
> LGTM
> 
> I have two concerns here:
> 
> 1) this is going to make it impossible for ads that abuse long window.names to
> mitigate their impact. So we'll have to figure out what we want to do there
Real
> Soon Now (tm). Charlie, if you have some time, let's chat real quickly and see
> if we're comfortable moving ahead with the hammer and throwing out backwards
> compat.

Yeah, let's find a chance to discuss this early next week.  (I see some activity
on that related email thread.)  I agree that it means ads can't later shorten
the frame name to reduce the memory impact, but I don't think we can sanely keep
this behavior given that it fundamentally breaks back/forward.  We'll need to
pursue another way to reduce the memory impact.  (I suppose the other unknown is
how we have to change the algorithm to fix https://crbug.com/500260.)

> 
> 2) subframe names that collided with the main frame's name will now have
> different unique names. This is probably OK... but something to be aware of. I
> guess we don't have any measurements of how common that is?

Good point.  Is it worth measuring what impact this would have, or is that too
obscure a case?  Seems pretty unlikely in practice.

> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html
> (right):
> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html:14:
> if (sessionStorage.alreadyRenamedSubframeAndTrigerredNavigations)
> Nit:triggered
> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/FrameTree.cpp (right):
> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/FrameTree.cpp:83: return;
> On 2016/09/09 16:21:38, Łukasz Anforowicz wrote:
> > I was wondering whether it might be a good idea to (in DCHECK_IS_ON builds?)
> > assign random garbage to m_uniqueName in case of a main frame (to
double-check
> > that nobody looks at the unique name of a main frame).
> 
> I think this is fine (and if they're using it, that's too bad)

[Łukasz Anforowicz, 2016-09-09 22:33:33]

Description was changed from

==========
Avoid mutating frame unique name after first real commit.

BUG=607205
==========

to

==========
Avoid mutating frame unique name after first real commit.

History navigations depend on frame unique names to identify frames that
need to load their state from history.  This identification is broken if
a frame's unique name can change throughout frame's lifetime.  For
example - the newly added layout tests fails (before FrameTree.cpp
changes introduced by this CL) to correctly navigate the subframe after
a back navigation.

BUG=607205
==========

[Łukasz Anforowicz, 2016-09-09 22:42:54]

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

[commit-bot: I haz the power, 2016-09-09 22:43:32]

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

[Łukasz Anforowicz, 2016-09-09 22:49:12]

I've removed main-frame-related changes and kicked off tryjobs - the smaller,
more focused change seems better (assuming the tryjobs pass).

On 2016/09/09 22:11:33, Charlie Reis (slow) wrote:
> Can you also add a bit of motivation to the CL description, covering both the
> subframe back/forward problem and the cleaner invariant we get with always
> having the main frame's unique name be empty?

Partially done (only covering the back/forward problem, because I started to
think that I should cover main frame unique name in a separate CL).

> On 2016/09/09 22:00:13, dcheng wrote:
> > LGTM
> > 
> > I have two concerns here:
> > 
> > 1) this is going to make it impossible for ads that abuse long window.names
to
> > mitigate their impact. So we'll have to figure out what we want to do there
> Real
> > Soon Now (tm). Charlie, if you have some time, let's chat real quickly and
see
> > if we're comfortable moving ahead with the hammer and throwing out backwards
> > compat.
> 
> Yeah, let's find a chance to discuss this early next week.  (I see some
activity
> on that related email thread.)  I agree that it means ads can't later shorten
> the frame name to reduce the memory impact, but I don't think we can sanely
keep
> this behavior given that it fundamentally breaks back/forward.  We'll need to
> pursue another way to reduce the memory impact.  (I suppose the other unknown
is
> how we have to change the algorithm to fix https://crbug.com/500260.)

Yes - I am curious what will happen in https://crbug.com/500260.  OTOH, I think
the current CL makes a progress in the right direction even without having
a resolution in https://crbug.com/500260, because:

1. The new layout test is useful to have regardless of the implementation
details.

2. I am not concerned about the changes in the current CL (after yanking out
   changes related to treatment of main frame).  The changes in the current CL
   are not really breaking, because previous behavior was already broken.

> > 2) subframe names that collided with the main frame's name will now have
> > different unique names. This is probably OK... but something to be aware of.
I
> > guess we don't have any measurements of how common that is?
> 
> Good point.  Is it worth measuring what impact this would have, or is that too
> obscure a case?  Seems pretty unlikely in practice.

It does seem to be an obscure case.  Additionally, this case would have led to
a duplicate unique name prior to https://codereview.chromium.org/1968213002.

Still - this is a valid point and I think I should at least move this aspect
of the changes to a separate CL.  When doing this I also need to remember
to update the doc comment (that does talk about the special case of main
frames).

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html
(right):

https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html:14:
if (sessionStorage.alreadyRenamedSubframeAndTrigerredNavigations)
On 2016/09/09 22:00:13, dcheng wrote:
> Nit:triggered

Done.

[dcheng, 2016-09-09 22:58:22]

On 2016/09/09 22:49:12, Łukasz Anforowicz wrote:
> I've removed main-frame-related changes and kicked off tryjobs - the smaller,
> more focused change seems better (assuming the tryjobs pass).
> 
> On 2016/09/09 22:11:33, Charlie Reis (slow) wrote:
> > Can you also add a bit of motivation to the CL description, covering both
the
> > subframe back/forward problem and the cleaner invariant we get with always
> > having the main frame's unique name be empty?
> 
> Partially done (only covering the back/forward problem, because I started to
> think that I should cover main frame unique name in a separate CL).
> 
> > On 2016/09/09 22:00:13, dcheng wrote:
> > > LGTM
> > > 
> > > I have two concerns here:
> > > 
> > > 1) this is going to make it impossible for ads that abuse long
window.names
> to
> > > mitigate their impact. So we'll have to figure out what we want to do
there
> > Real
> > > Soon Now (tm). Charlie, if you have some time, let's chat real quickly and
> see
> > > if we're comfortable moving ahead with the hammer and throwing out
backwards
> > > compat.
> > 
> > Yeah, let's find a chance to discuss this early next week.  (I see some
> activity
> > on that related email thread.)  I agree that it means ads can't later
shorten
> > the frame name to reduce the memory impact, but I don't think we can sanely
> keep
> > this behavior given that it fundamentally breaks back/forward.  We'll need
to
> > pursue another way to reduce the memory impact.  (I suppose the other
unknown
> is
> > how we have to change the algorithm to fix https://crbug.com/500260.)
> 
> Yes - I am curious what will happen in https://crbug.com/500260.  OTOH, I
think
> the current CL makes a progress in the right direction even without having
> a resolution in https://crbug.com/500260, because:
> 
> 1. The new layout test is useful to have regardless of the implementation
> details.
> 
> 2. I am not concerned about the changes in the current CL (after yanking out
>    changes related to treatment of main frame).  The changes in the current CL
>    are not really breaking, because previous behavior was already broken.

Right, I don't think this is blocking, but it does mean we should raise the
priority of figuring out what to do with the long unique name case.

> 
> > > 2) subframe names that collided with the main frame's name will now have
> > > different unique names. This is probably OK... but something to be aware
of.
> I
> > > guess we don't have any measurements of how common that is?
> > 
> > Good point.  Is it worth measuring what impact this would have, or is that
too
> > obscure a case?  Seems pretty unlikely in practice.
> 
> It does seem to be an obscure case.  Additionally, this case would have led to
> a duplicate unique name prior to https://codereview.chromium.org/1968213002.
> 
> Still - this is a valid point and I think I should at least move this aspect
> of the changes to a separate CL.  When doing this I also need to remember
> to update the doc comment (that does talk about the special case of main
> frames).
> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html
> (right):
> 
>
https://codereview.chromium.org/2317203002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/navigation/rename-subframe-goback.html:14:
> if (sessionStorage.alreadyRenamedSubframeAndTrigerredNavigations)
> On 2016/09/09 22:00:13, dcheng wrote:
> > Nit:triggered
> 
> Done.

[Łukasz Anforowicz, 2016-09-13 18:02:24]

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

[commit-bot: I haz the power, 2016-09-13 18:03:06]

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-13 20:11:59]

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

[commit-bot: I haz the power, 2016-09-13 20:12:01]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-13 20:21:38]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-13 20:21:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2317203002/#ps120001
(title: "Tweaking the comment describing the format of unique name.")

[commit-bot: I haz the power, 2016-09-13 20:22:02]

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-13 20:28:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-13 20:30:41]

Description was changed from

==========
Avoid mutating frame unique name after first real commit.

History navigations depend on frame unique names to identify frames that
need to load their state from history.  This identification is broken if
a frame's unique name can change throughout frame's lifetime.  For
example - the newly added layout tests fails (before FrameTree.cpp
changes introduced by this CL) to correctly navigate the subframe after
a back navigation.

BUG=607205
==========

to

==========
Avoid mutating frame unique name after first real commit.

History navigations depend on frame unique names to identify frames that
need to load their state from history.  This identification is broken if
a frame's unique name can change throughout frame's lifetime.  For
example - the newly added layout tests fails (before FrameTree.cpp
changes introduced by this CL) to correctly navigate the subframe after
a back navigation.

BUG=607205

Committed: https://crrev.com/2ae993fad864e5c683d009ebdfbb0bdae3faef44
Cr-Commit-Position: refs/heads/master@{#418350}
==========

[commit-bot: I haz the power, 2016-09-13 20:30:42]

Patchset 7 (id:??) landed as
https://crrev.com/2ae993fad864e5c683d009ebdfbb0bdae3faef44
Cr-Commit-Position: refs/heads/master@{#418350}