Main frame's unique name can always be null.

Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=647392
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5140a41c48c0932e9a2db3f969ce43393ae0dbef
Cr-Commit-Position: refs/heads/master@{#418976}

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

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

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

Daniel + Charlie, do you think it is worth it to have main frame's unique name
always remain set to null?

There is a tiny chance of breaking session history's backcompatibility that
Daniel brought up in https://codereview.chromium.org/2317203002/#msg19, but it
seem to be an obscure case and additionally, this case would have led to a
duplicate unique name prior to https://codereview.chromium.org/1968213002.

WDYT?

PS. I'll open a bug and refer to it in the CL description if you think this is
something we want to do.

PPS. I hope the trybots will be appreciative and will turn green (this patchset
should be more or less equivalent to patchset #5 in
https://codereview.chromium.org/2317203002/#ps80001).

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

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

[commit-bot: I haz the power, 2016-09-09 23:39:00]

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-10 01:09:42]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-10 02:54:49]

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
Is this non-empty sometimes for parent frames? I didn't read around that much,
but it seems new views are always create with no name?

[Charlie Reis, 2016-09-12 20:27:37]

I do think this would be nice to have.  The compat risk seems pretty low given
your observation, and it gives us fewer cases to reason about.  (Some potential
for memory improvements as well, though I don't think main frames tended to be
the ones with enormous names.)

[Łukasz Anforowicz, 2016-09-12 23:58:35]

On 2016/09/12 20:27:37, Charlie Reis (slow) wrote:
> I do think this would be nice to have.  The compat risk seems pretty low given
> your observation, and it gives us fewer cases to reason about.

Ack.

> (Some potential
> for memory improvements as well, though I don't think main frames tended to be
> the ones with enormous names.)

Right.  Additionally, in absence of conflicts, |m_uniqueName| should be equal to
|m_name| and AtomicString should already dedupe the string between these 2
fields (so I think that [in absence of uniqueName conflicts] this CL should have
no impact on memory consumption).

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
On 2016/09/10 02:54:49, dcheng wrote:
> Is this non-empty sometimes for parent frames? I didn't read around that much,
> but it seems new views are always create with no name?

I am not sure if I understand the question, but I think you are asking if new
views can be named right from the start.  AFAICT new views can be named by:

1. window.open(url, window_name) [+ I see that FrameTree::setName is called on
the main frame from createNewWindow function in CreateWindow.cpp]

2. embedder [i.e. I see that ViewMsg_New_Params contains
replicated_frame_state.name + there is
content::WebContents::CreateParams::main_frame_name]

[dcheng, 2016-09-13 00:44:24]

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
On 2016/09/12 23:58:34, Łukasz Anforowicz wrote:
> On 2016/09/10 02:54:49, dcheng wrote:
> > Is this non-empty sometimes for parent frames? I didn't read around that
much,
> > but it seems new views are always create with no name?
> 
> I am not sure if I understand the question, but I think you are asking if new
> views can be named right from the start.  AFAICT new views can be named by:
> 
> 1. window.open(url, window_name) [+ I see that FrameTree::setName is called on
> the main frame from createNewWindow function in CreateWindow.cpp]
> 
> 2. embedder [i.e. I see that ViewMsg_New_Params contains
> replicated_frame_state.name + there is
> content::WebContents::CreateParams::main_frame_name]

Right, but I don't think the main frame is ever *created* with a name set. In
the window.open case, the name isn't set until later (when we return back out of
content into Blink). In the IPC case, it's not set until a later explicit call
to FrameTree::setName. So I'm wondering if we ever call setPrecalculatedName for
a main frame and |name| won't be null / empty.

[Łukasz Anforowicz, 2016-09-13 01:14:29]

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
On 2016/09/13 00:44:23, dcheng wrote:
> On 2016/09/12 23:58:34, Łukasz Anforowicz wrote:
> > On 2016/09/10 02:54:49, dcheng wrote:
> > > Is this non-empty sometimes for parent frames? I didn't read around that
> much,
> > > but it seems new views are always create with no name?
> > 
> > I am not sure if I understand the question, but I think you are asking if
new
> > views can be named right from the start.  AFAICT new views can be named by:
> > 
> > 1. window.open(url, window_name) [+ I see that FrameTree::setName is called
on
> > the main frame from createNewWindow function in CreateWindow.cpp]
> > 
> > 2. embedder [i.e. I see that ViewMsg_New_Params contains
> > replicated_frame_state.name + there is
> > content::WebContents::CreateParams::main_frame_name]
> 
> Right, but I don't think the main frame is ever *created* with a name set.

Agreed.

> In the window.open case, the name isn't set until later (when we return back
out of
> content into Blink). In the IPC case, it's not set until a later explicit call
> to FrameTree::setName.

Ack.

> So I'm wondering if we ever call setPrecalculatedName for
> a main frame and |name| won't be null / empty.

Yes - we would use setPrecalculatedName when replicating unique name to another
renderer process (i.e. via WebRemoteFrameImpl::setReplicatedName and [for
example] RenderFrameProxy::OnDidUpdateName).  In this case name and unique name
might not be null / empty.

[Łukasz Anforowicz, 2016-09-13 01:18:14]

On 2016/09/12 23:58:35, Łukasz Anforowicz wrote:
> On 2016/09/12 20:27:37, Charlie Reis (slow) wrote:
> > I do think this would be nice to have.  The compat risk seems pretty low
given
> > your observation, and it gives us fewer cases to reason about.
> 
> Ack.
> 
> > (Some potential
> > for memory improvements as well, though I don't think main frames tended to
be
> > the ones with enormous names.)
> 
> Right.  Additionally, in absence of conflicts, |m_uniqueName| should be equal
to
> |m_name| and AtomicString should already dedupe the string between these 2
> fields (so I think that [in absence of uniqueName conflicts] this CL should
have
> no impact on memory consumption).

Although I guess AtomicString wouldn't help after uniqueName diverges from name
after the CL that makes uniqueName immutable after first real commit.  This
makes me
convinced that this CL is desirable not only as a code quality improvement,
but also to help reduce memory consumption by a tiny bit.

[dcheng, 2016-09-13 01:38:00]

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
On 2016/09/13 01:14:29, Łukasz Anforowicz wrote:
> On 2016/09/13 00:44:23, dcheng wrote:
> > On 2016/09/12 23:58:34, Łukasz Anforowicz wrote:
> > > On 2016/09/10 02:54:49, dcheng wrote:
> > > > Is this non-empty sometimes for parent frames? I didn't read around that
> > much,
> > > > but it seems new views are always create with no name?
> > > 
> > > I am not sure if I understand the question, but I think you are asking if
> new
> > > views can be named right from the start.  AFAICT new views can be named
by:
> > > 
> > > 1. window.open(url, window_name) [+ I see that FrameTree::setName is
called
> on
> > > the main frame from createNewWindow function in CreateWindow.cpp]
> > > 
> > > 2. embedder [i.e. I see that ViewMsg_New_Params contains
> > > replicated_frame_state.name + there is
> > > content::WebContents::CreateParams::main_frame_name]
> > 
> > Right, but I don't think the main frame is ever *created* with a name set.
> 
> Agreed.
> 
> > In the window.open case, the name isn't set until later (when we return back
> out of
> > content into Blink). In the IPC case, it's not set until a later explicit
call
> > to FrameTree::setName.
> 
> Ack.
> 
> > So I'm wondering if we ever call setPrecalculatedName for
> > a main frame and |name| won't be null / empty.
> 
> Yes - we would use setPrecalculatedName when replicating unique name to
another
> renderer process (i.e. via WebRemoteFrameImpl::setReplicatedName and [for
> example] RenderFrameProxy::OnDidUpdateName).  In this case name and unique
name
> might not be null / empty.

Right, but the unique name we calculated in that case ought to be empty anyway.
Basically, what I'm wondering: if we didn't have this if () statement, what
would break?

[Łukasz Anforowicz, 2016-09-13 15:43:59]

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

[commit-bot: I haz the power, 2016-09-13 15:44:20]

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 16:25:36]

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

[commit-bot: I haz the power, 2016-09-13 16:25:37]

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

[Łukasz Anforowicz, 2016-09-13 16:45:14]

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

https://codereview.chromium.org/2329523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:99: if (!parent())
On 2016/09/13 01:38:00, dcheng wrote:
> On 2016/09/13 01:14:29, Łukasz Anforowicz wrote:
> > On 2016/09/13 00:44:23, dcheng wrote:
> > > On 2016/09/12 23:58:34, Łukasz Anforowicz wrote:
> > > > On 2016/09/10 02:54:49, dcheng wrote:
> > > > > Is this non-empty sometimes for parent frames? I didn't read around
that
> > > much,
> > > > > but it seems new views are always create with no name?
> > > > 
> > > > I am not sure if I understand the question, but I think you are asking
if
> > new
> > > > views can be named right from the start.  AFAICT new views can be named
> by:
> > > > 
> > > > 1. window.open(url, window_name) [+ I see that FrameTree::setName is
> called
> > on
> > > > the main frame from createNewWindow function in CreateWindow.cpp]
> > > > 
> > > > 2. embedder [i.e. I see that ViewMsg_New_Params contains
> > > > replicated_frame_state.name + there is
> > > > content::WebContents::CreateParams::main_frame_name]
> > > 
> > > Right, but I don't think the main frame is ever *created* with a name set.
> > 
> > Agreed.
> > 
> > > In the window.open case, the name isn't set until later (when we return
back
> > out of
> > > content into Blink). In the IPC case, it's not set until a later explicit
> call
> > > to FrameTree::setName.
> > 
> > Ack.
> > 
> > > So I'm wondering if we ever call setPrecalculatedName for
> > > a main frame and |name| won't be null / empty.
> > 
> > Yes - we would use setPrecalculatedName when replicating unique name to
> another
> > renderer process (i.e. via WebRemoteFrameImpl::setReplicatedName and [for
> > example] RenderFrameProxy::OnDidUpdateName).  In this case name and unique
> name
> > might not be null / empty.
> 
> Right, but the unique name we calculated in that case ought to be empty
anyway.
> Basically, what I'm wondering: if we didn't have this if () statement, what
> would break?

It turns out [1] that when setPrecalculatedName is called during swap-out [2],
then the parent is not yet set, but obviously the unique name (coming from a
subframe / non-main-frame) can be not empty.

So:
- The early return is wrong / would mess up unique names during swap-out.  I
probably should spend some time coming up with extra test coverage (because the
first patchset should not be green I think).
- The last patchset contains corrected FrameTree.cpp changes.

[1] https://codereview.chromium.org/2329523003/#ps20001 and
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[2] #2 0x000003d309aa blink::FrameTree::setPrecalculatedName()@@@
#3 0x00000328fd54 blink::WebRemoteFrameImpl::setReplicatedName()@@@
#4 0x000002f283f5 content::RenderFrameProxy::SetReplicatedState()@@@
#5 0x000002f051ef content::RenderFrameImpl::OnSwapOut()@@@

[Łukasz Anforowicz, 2016-09-13 16:46:55]

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

[commit-bot: I haz the power, 2016-09-13 16:47:24]

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 18:04:26]

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

[commit-bot: I haz the power, 2016-09-13 18:04:27]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-14 17:26:24]

Description was changed from

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=
==========

to

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-09-14 17:26:29]

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

[commit-bot: I haz the power, 2016-09-14 17:26:56]

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

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

[commit-bot: I haz the power, 2016-09-14 20:29:44]

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-14 22:08:42]

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

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

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-14 22:20:01]

Daniel, can you take a look at patchset #6?

[dcheng, 2016-09-15 06:33:23]

LGTM with nits

https://codereview.chromium.org/2329523003/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2329523003/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1543: std::string unique_name
= "";
Omit "" and just default initialize (it's slightly more efficient)

https://codereview.chromium.org/2329523003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2329523003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:351: DCHECK(parent()); // Only
subframes can have a custom-tailored unique name.
custom-tailored => non-empty

[Łukasz Anforowicz, 2016-09-15 15:47:48]

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

[commit-bot: I haz the power, 2016-09-15 15:48:04]

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-15 15:55:18]

Charlie, can you do an OWNERS review for //content?

https://codereview.chromium.org/2329523003/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2329523003/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1543: std::string unique_name
= "";
On 2016/09/15 06:33:23, dcheng wrote:
> Omit "" and just default initialize (it's slightly more efficient)

Done.

https://codereview.chromium.org/2329523003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2329523003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:351: DCHECK(parent()); // Only
subframes can have a custom-tailored unique name.
On 2016/09/15 06:33:23, dcheng wrote:
> custom-tailored => non-empty

Done + expanded the comment a bit more.

[commit-bot: I haz the power, 2016-09-15 17:00:25]

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

[commit-bot: I haz the power, 2016-09-15 17:00:26]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-15 20:04:12]

content/ LGTM.  Thanks!  (I think I'll probably be able to do some
NavigationEntryImpl cleanup as a result, but we don't need to address that
here.)

Maybe file a bug just so we have something to reference in the CL description,
in case there's an unexpected regression.

https://codereview.chromium.org/2329523003/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2329523003/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:241: DCHECK(unique_name.empty());
This is in response to an IPC, right?  Should we enforce this in any way for
release builds, like clearing the unique_name if it isn't empty for a main
frame?

[Łukasz Anforowicz, 2016-09-15 20:28:40]

Description was changed from

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=647392
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-09-15 20:55:13]

On 2016/09/15 20:04:12, Charlie Reis (slow) wrote:
> content/ LGTM.  Thanks!  (I think I'll probably be able to do some
> NavigationEntryImpl cleanup as a result, but we don't need to address that
> here.)

Ack.
> 
> Maybe file a bug just so we have something to reference in the CL description,
> in case there's an unexpected regression.

Done - I've opened http://crbug.com/647392 + updated the CL description.

https://codereview.chromium.org/2329523003/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2329523003/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:241: DCHECK(unique_name.empty());
On 2016/09/15 20:04:12, Charlie Reis (slow) wrote:
> This is in response to an IPC, right?

Usually yes, but not necessarily.  For example - a non-IPC call can be made from
the place I had to fix-up in WebContentsImpl::Init.

> Should we enforce this in any way for
> release builds, like clearing the unique_name if it isn't empty for a main
> frame?

I am not sure.  I think doing this for emptiness of main frame is not needed:

1. Browser should not look at the unique name of the main frame.

2. Old code worked fine when unique name might have been non-empty.

3. The DCHECKs I've added are mainly to help me^H^H^H future maintainers avoid
the mistake I've made in patchset #1 (where I've modified the unique name format
in Blink, but forgot about this one place in browser-side).


I guess some browser-side validation might still be desirable - most importantly
to make sure the unique names are truly unique (AFAIR we had a security bug
where duplicate unique names where one part of a bigger repro/puzzle).  Still -
even non-malicious renderers can hit a race and generate a non-unique name, so I
don't see how to do the enforcement here (we can't kill a renderer, but there is
no clear fallback).

[Łukasz Anforowicz, 2016-09-15 21:05:08]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-15 21:05:08]

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/2329523003/#ps120001
(title: "Addressed CR feedback from dcheng@.")

[commit-bot: I haz the power, 2016-09-15 21:05:44]

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-15 21:12:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-15 21:16:29]

Description was changed from

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=647392
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Main frame's unique name can always be null.

This simplifies the code a little bit + avoids consuming memory for what
is ultimately an unused value.

BUG=647392
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5140a41c48c0932e9a2db3f969ce43393ae0dbef
Cr-Commit-Position: refs/heads/master@{#418976}
==========

[commit-bot: I haz the power, 2016-09-15 21:16:30]

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