Returning early when setting a frame name to the same name.

Returning early when setting a frame name to the same name.

BUG=618813
Committed: https://crrev.com/d21ac9cb13ff2eac7f141bc04b3ed9d432c6f7ee
Cr-Commit-Position: refs/heads/master@{#399988}

[Łukasz Anforowicz, 2016-06-10 00:06:56]

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

[Łukasz Anforowicz, 2016-06-10 00:06:57]

Daniel, can you take a look please?

https://codereview.chromium.org/2053903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2053903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1060: return;
Alex rightfully suggested that we should also short-circuit here (to avoid
unnecessary IPC to the browser).

Note that even with the early return above, we still need to tweak
FrameTree::setName, because it has more callers than just
LocalDOMWindow::setName.

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

https://codereview.chromium.org/2053903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:62: if (m_name == name &&
!m_uniqueName.isNull())
This would be really easy to understand if this could just say |m_name == name|.
 I think this is not sufficient, because FrameTree constructor constructs a
half-baked object - it assumes that callers will eventually call setName or
setPrecalculatedName.  In such cases, we still want to generate a fresh
m_uniqueName (even if m_name == name - e.g. if name == null).

I've convinced myself that the condition above correctly handles the case of
m_uniqueName.isNull().  For non-main-frame, m_uniqueName can be null only if
unique name has not yet been initialized.  For main frame we allow a null or
empty m_uniqueName, so m_uniqueName can be null if 1) old frame name was null 
or 2) unique name has not yet been initialized.  For (2) we obviously want to
generate a unique case, but generating for (1) is also okay - we will always
generate null unique-name if main frame's assigned-name is null.

I don't see any better solution:

- We could distinguish between m_uniqueName being null and empty.  I don't think
this is any less complex.

- We could audit all calls to FrameTree::setName and check in what conditions
the call is made (only made for main frame?  only made right after creating the
frame?), but this seems fragile / unmaintainable.

[dcheng, 2016-06-10 23:51:14]

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

https://codereview.chromium.org/2053903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:62: if (m_name == name &&
!m_uniqueName.isNull())
On 2016/06/10 00:06:57, Łukasz Anforowicz wrote:
> This would be really easy to understand if this could just say |m_name ==
name|.
>  I think this is not sufficient, because FrameTree constructor constructs a
> half-baked object - it assumes that callers will eventually call setName or
> setPrecalculatedName.  In such cases, we still want to generate a fresh
> m_uniqueName (even if m_name == name - e.g. if name == null).
> 
> I've convinced myself that the condition above correctly handles the case of
> m_uniqueName.isNull().  For non-main-frame, m_uniqueName can be null only if
> unique name has not yet been initialized.  For main frame we allow a null or
> empty m_uniqueName, so m_uniqueName can be null if 1) old frame name was null 
> or 2) unique name has not yet been initialized.  For (2) we obviously want to
> generate a unique case, but generating for (1) is also okay - we will always
> generate null unique-name if main frame's assigned-name is null.
> 
> I don't see any better solution:
> 
> - We could distinguish between m_uniqueName being null and empty.  I don't
think
> this is any less complex.
> 
> - We could audit all calls to FrameTree::setName and check in what conditions
> the call is made (only made for main frame?  only made right after creating
the
> frame?), but this seems fragile / unmaintainable.

As we discussed, let's try to elaborate this comment a little more on why this
happens: I guess this is happening when creating a subframe with no default
name? But we should clarify that. Where is setName() getting called in that init
path? etc.

[Łukasz Anforowicz, 2016-06-14 23:58:43]

Daniel, can you take another look please?

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

https://codereview.chromium.org/2053903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:62: if (m_name == name &&
!m_uniqueName.isNull())
On 2016/06/10 23:51:14, dcheng wrote:
> On 2016/06/10 00:06:57, Łukasz Anforowicz wrote:
> > This would be really easy to understand if this could just say |m_name ==
> name|.
> >  I think this is not sufficient, because FrameTree constructor constructs a
> > half-baked object - it assumes that callers will eventually call setName or
> > setPrecalculatedName.  In such cases, we still want to generate a fresh
> > m_uniqueName (even if m_name == name - e.g. if name == null).
> > 
> > I've convinced myself that the condition above correctly handles the case of
> > m_uniqueName.isNull().  For non-main-frame, m_uniqueName can be null only if
> > unique name has not yet been initialized.  For main frame we allow a null or
> > empty m_uniqueName, so m_uniqueName can be null if 1) old frame name was
null 
> > or 2) unique name has not yet been initialized.  For (2) we obviously want
to
> > generate a unique case, but generating for (1) is also okay - we will always
> > generate null unique-name if main frame's assigned-name is null.
> > 
> > I don't see any better solution:
> > 
> > - We could distinguish between m_uniqueName being null and empty.  I don't
> think
> > this is any less complex.
> > 
> > - We could audit all calls to FrameTree::setName and check in what
conditions
> > the call is made (only made for main frame?  only made right after creating
> the
> > frame?), but this seems fragile / unmaintainable.

Unless there is an assert, as in patchset #3?  Hmmm....
> 
> As we discussed, let's try to elaborate this comment a little more on why this
> happens: I guess this is happening when creating a subframe with no default
> name? But we should clarify that. Where is setName() getting called in that
init
> path? etc.

I've expanded the comments, but when researching this, I've found out that
setName should never be called on an uninitialized FrameTree that belongs to a
subframe.  So - in the end, I decided to simplify the condition above by
replacing the second part with an assert.

Expanded comments: patchset #2
Simplified condition: patchset #3

[dcheng, 2016-06-15 11:18:35]

LGTM with an optional nit.

https://codereview.chromium.org/2053903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2053903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1064:
frame()->loader().client()->didChangeName(name, frame()->tree().uniqueName());
I'd suggest moving this into FrameTree::setName() so we don't have to duplicate
the check.

[Łukasz Anforowicz, 2016-06-15 17:45:34]

Patchset #4 (id:60001) has been deleted

[Łukasz Anforowicz, 2016-06-15 17:46:22]

Thanks for the review Daniel.

https://codereview.chromium.org/2053903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2053903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1064:
frame()->loader().client()->didChangeName(name, frame()->tree().uniqueName());
On 2016/06/15 11:18:35, dcheng wrote:
> I'd suggest moving this into FrameTree::setName() so we don't have to
duplicate
> the check.

I think this is a great idea.  At the same time, I'd like to do this in a
separate CL.

I was guessing that some calls to FrameTree::setName would not propagate back to
the browser - moving the FrameLoaderClient notification to FrameTree::setName
should plug this hole.  OTOH, this probably needs some tests to show that there
actually is a problem (and therefore I'd like to do this in a separate CL). 
FWIW, I couldn't repro this problem yet (I thought that the setName call in
createNewWindow would not be propagated).

I also need to note that moving client notification can result in unnecessary
notifications in cases when 1) the browser already knows the new name, but 2)
FrameTree name has not been setup yet.  For such cases:

- We should still have correct, same behavior (because such unnecessary IPCs
should be ignored by FrameTreeNode::SetFrameName (after seeing new_name ==
old_name).

- In the long-term such cases should go through FrameTree::setPrecalculatedName
instead of FrameTree::setName (but this is tricky because the format is only
exposed through Blink's "core" layer, not through "web" layer).

[Łukasz Anforowicz, 2016-06-15 17:46:48]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-15 17:48:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2053903002/40001

[commit-bot: I haz the power, 2016-06-15 17:55:32]

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

[commit-bot: I haz the power, 2016-06-15 17:55:34]

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

[Łukasz Anforowicz, 2016-06-15 18:42:35]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-15 18:43:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2053903002/40001

[commit-bot: I haz the power, 2016-06-15 19:31:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-15 19:32:38]

Description was changed from

==========
Returning early when setting a frame name to the same name.

BUG=618813
==========

to

==========
Returning early when setting a frame name to the same name.

BUG=618813

Committed: https://crrev.com/d21ac9cb13ff2eac7f141bc04b3ed9d432c6f7ee
Cr-Commit-Position: refs/heads/master@{#399988}
==========

[commit-bot: I haz the power, 2016-06-15 19:32:39]

Patchset 3 (id:??) landed as
https://crrev.com/d21ac9cb13ff2eac7f141bc04b3ed9d432c6f7ee
Cr-Commit-Position: refs/heads/master@{#399988}