Avoiding going through RenderFrameProxy when targeting a *new* frame.

Avoiding going through RenderFrameProxy when targeting a *new* frame.

BUG=647772
Committed: https://crrev.com/e7250ebe260957a67131667cdde6b7b838a9b1f5
Cr-Commit-Position: refs/heads/master@{#426483}

[Łukasz Anforowicz, 2016-10-12 23:57:21]

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

[commit-bot: I haz the power, 2016-10-12 23:57:47]

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-10-13 00:02:40]

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

[commit-bot: I haz the power, 2016-10-13 00:02:46]

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-10-13 00:07:13]

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

[commit-bot: I haz the power, 2016-10-13 00:07:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Łukasz Anforowicz, 2016-10-13 01:53:48]

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

[commit-bot: I haz the power, 2016-10-13 01:54: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-10-13 03:44:16]

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

[commit-bot: I haz the power, 2016-10-13 03:44:17]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-13 14:08:00]

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

[Łukasz Anforowicz, 2016-10-13 14:08:01]

japhet@, could you please take a look?

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1068: Frame* targetFrame =
request.form()
I don't quite understand the reasoning behind making an exception for
|request.form|.  FWIW, I am keeping this exception in the current CL under
review, so this should be fine.

But... I wonder if this exception is in any way related to lack of support for
POST requests in OpenURL path (which should be fixed now) and related to
avoiding ShouldFork for POST requests (not yet fixed;  work tracked by
https://crbug.com/650694).  If yes, then maybe I could look into removing
special handling of forms (here and also in shouldOpenInNewWindow).

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1105: }
Keeping |targetFrame| set to nullptr in case of Ctrl-Click / Shift-Click helps
fix https://crbug.com/647772, because it:

1) continues to work as expected (because the navigation targets a *new*
frame/tab, not |targetFrame|)

2) avoids going through RenderFrameProxy when |targetFrame| is a remote frame; 
before this CL, this would have happened below inside |if (targetFrame &&
targetFrame != m_frame)|.

[Nate Chapin, 2016-10-13 22:11:24]

This seems pretty reasonable, I just want to see if we can leave the code
cleaner than we found it :)

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1068: Frame* targetFrame =
request.form()
On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> I don't quite understand the reasoning behind making an exception for
|request.form|.  FWIW, I am keeping this exception in the current CL under
review, so this should be fine.
> 
> But... I wonder if this exception is in any way related to lack of support for
POST requests in OpenURL path (which should be fixed now) and related to
avoiding ShouldFork for POST requests (not yet fixed;  work tracked by
https://crbug.com/650694).  If yes, then maybe I could look into removing
special handling of forms (here and also in shouldOpenInNewWindow).

shouldOpenInNewWindow() excluding forms is definitely 650694, and I think this
case is, too.

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1105: }
On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> Keeping |targetFrame| set to nullptr in case of Ctrl-Click / Shift-Click helps
fix https://crbug.com/647772, because it:
> 
> 1) continues to work as expected (because the navigation targets a *new*
frame/tab, not |targetFrame|)
> 
> 2) avoids going through RenderFrameProxy when |targetFrame| is a remote frame;
 before this CL, this would have happened below inside |if (targetFrame &&
targetFrame != m_frame)|.

Ideally, the shouldOpenInNewWindow() case would be in the same location as this
logic. Can we put them together, and if so, does that allow for any
simplifications?

[Łukasz Anforowicz, 2016-10-15 01:29:39]

My apologies for not replying earlier (I needed to look at another bug).  I'll
try to apply your suggestions and get back to you early next week.

[Łukasz Anforowicz, 2016-10-17 21:10:34]

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

[commit-bot: I haz the power, 2016-10-17 21:11:37]

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-10-17 22:57:53]

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

[commit-bot: I haz the power, 2016-10-17 22:57:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2016-10-18 16:48:15]

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

[commit-bot: I haz the power, 2016-10-18 16:48:46]

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-10-18 19:40:59]

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

[commit-bot: I haz the power, 2016-10-18 19:41:00]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-18 23:13:17]

japhet@, could you please take another look?  Unfortunately, I don't see how to
clean-up or combine form-related workarounds, without bumping into a bunch of
issues (please see my comments below).

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1068: Frame* targetFrame =
request.form()
On 2016/10/13 22:11:23, Nate Chapin wrote:
> On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> > I don't quite understand the reasoning behind making an exception for
> |request.form|.  FWIW, I am keeping this exception in the current CL under
> review, so this should be fine.
> > 
> > But... I wonder if this exception is in any way related to lack of support
for
> POST requests in OpenURL path (which should be fixed now) and related to
> avoiding ShouldFork for POST requests (not yet fixed;  work tracked by
> https://crbug.com/650694).  If yes, then maybe I could look into removing
> special handling of forms (here and also in shouldOpenInNewWindow).
> 
> shouldOpenInNewWindow() excluding forms is definitely 650694

Actually, we don't need to fix https://crbug.com/650694 in order to remove the
workaround.  POST no longer downgrades to a GET for quite a while already (since
June) - this has been fixed in my r400442.  Unfortunately, there are still some
blockers for removing the workaround:
- content_shell doesn't support window dispositions other than CURRENT_TAB,
which breaks http/tests/navigation/post-with-modifier.html layout test
- even after adding content_shell support for NEW_WINDOW disposition, the test
failed, because of the issue pointed out in a comment in
https://crbug.com/606594#c1.  This is a hairy issue to fix (I am trying to make
some, but rather slow progress in https://codereview.chromium.org/2349063002/
and https://codereview.chromium.org/1814963002/).

I am wondering whether it might make sense to replace
http/tests/navigation/post-with-modifier.html with a browser test instead. 
WDYT?

> and I think this case is, too.

Maybe.  I haven't yet looked into this too much.

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1105: }
On 2016/10/13 22:11:23, Nate Chapin wrote:
> On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> > Keeping |targetFrame| set to nullptr in case of Ctrl-Click / Shift-Click
helps
> fix https://crbug.com/647772, because it:
> > 
> > 1) continues to work as expected (because the navigation targets a *new*
> frame/tab, not |targetFrame|)
> > 
> > 2) avoids going through RenderFrameProxy when |targetFrame| is a remote
frame;
>  before this CL, this would have happened below inside |if (targetFrame &&
> targetFrame != m_frame)|.
> 
> Ideally, the shouldOpenInNewWindow() case would be in the same location as
this
> logic. Can we put them together, and if so, does that allow for any
> simplifications?

Well, both shouldOpenInNewWindow as well as the |if (targetFrame && targetFrame
!= m_frame)| check are impacted by special-casing forms (and these 2 places are
the only consumers of |targetFrame| variable).

Still, I tried various changes described in
https://docs.google.com/document/d/1fQIsKlua8aMUQhH7PvKOBlHxJsO9xR2KS0TjTZd-oIE
and didn't get very far (both - the changes are not a big improvement + I
couldn't get all the tests to pass).  I am starting to think that even if there
are refactoring opportunities here, we should do them in a separate CL (to keep
the CL under review simple + to avoid the risk of it getting reverted because it
bundles in some extra, opportunistic changes).

FWIW, going through the exercise of breaking down various conditions helped me
catch one issue (see
https://docs.google.com/document/d/1fQIsKlua8aMUQhH7PvKOBlHxJsO9xR2KS0TjTZd-o...)
and fix it in the latest patchset.

PS. In the CL, I did end up moving things around, so that variables are
introduced right next to the place where they are needed.  I guess this is not
quite the kind of cleanup you were hoping for... :-/

[dcheng, 2016-10-18 23:18:34]

drive-by

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:972: static bool
canIgnoreTargetFrame(NavigationPolicy policy) {
Minor nit: shouldIgnoreTargetFrame() seems more consistent with the local naming
convention.

[Łukasz Anforowicz, 2016-10-18 23:26:45]

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:972: static bool
canIgnoreTargetFrame(NavigationPolicy policy) {
On 2016/10/18 23:18:34, dcheng wrote:
> Minor nit: shouldIgnoreTargetFrame() seems more consistent with the local
naming
> convention.

Thanks for pointing this out.  I think this function answers "can ignore" rather
than "should ignore" question - ignoring is optional for local frames (the
behavior will be the same regardless of which frame we go through);  ignoring is
mandatory (i.e. a "should") only if the target is a remote frame (which doesn't
handle window dispositions other than CURRENT_TAB).

[Nate Chapin, 2016-10-18 23:37:22]

blink lgtm

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1068: Frame* targetFrame =
request.form()
On 2016/10/18 23:13:17, Łukasz Anforowicz wrote:
> On 2016/10/13 22:11:23, Nate Chapin wrote:
> > On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> > > I don't quite understand the reasoning behind making an exception for
> > |request.form|.  FWIW, I am keeping this exception in the current CL under
> > review, so this should be fine.
> > > 
> > > But... I wonder if this exception is in any way related to lack of support
> for
> > POST requests in OpenURL path (which should be fixed now) and related to
> > avoiding ShouldFork for POST requests (not yet fixed;  work tracked by
> > https://crbug.com/650694).  If yes, then maybe I could look into removing
> > special handling of forms (here and also in shouldOpenInNewWindow).
> > 
> > shouldOpenInNewWindow() excluding forms is definitely 650694
> 
> Actually, we don't need to fix https://crbug.com/650694 in order to remove the
> workaround.  POST no longer downgrades to a GET for quite a while already
(since
> June) - this has been fixed in my r400442.  Unfortunately, there are still
some
> blockers for removing the workaround:
> - content_shell doesn't support window dispositions other than CURRENT_TAB,
> which breaks http/tests/navigation/post-with-modifier.html layout test
> - even after adding content_shell support for NEW_WINDOW disposition, the test
> failed, because of the issue pointed out in a comment in
> https://crbug.com/606594#c1.  This is a hairy issue to fix (I am trying to
make
> some, but rather slow progress in https://codereview.chromium.org/2349063002/
> and https://codereview.chromium.org/1814963002/).
> 
> I am wondering whether it might make sense to replace
> http/tests/navigation/post-with-modifier.html with a browser test instead. 
> WDYT?

Sounds like a good idea.

> 
> > and I think this case is, too.
> 
> Maybe.  I haven't yet looked into this too much.

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1105: }
On 2016/10/18 23:13:17, Łukasz Anforowicz wrote:
> On 2016/10/13 22:11:23, Nate Chapin wrote:
> > On 2016/10/13 at 14:08:01, Łukasz Anforowicz wrote:
> > > Keeping |targetFrame| set to nullptr in case of Ctrl-Click / Shift-Click
> helps
> > fix https://crbug.com/647772, because it:
> > > 
> > > 1) continues to work as expected (because the navigation targets a *new*
> > frame/tab, not |targetFrame|)
> > > 
> > > 2) avoids going through RenderFrameProxy when |targetFrame| is a remote
> frame;
> >  before this CL, this would have happened below inside |if (targetFrame &&
> > targetFrame != m_frame)|.
> > 
> > Ideally, the shouldOpenInNewWindow() case would be in the same location as
> this
> > logic. Can we put them together, and if so, does that allow for any
> > simplifications?
> 
> Well, both shouldOpenInNewWindow as well as the |if (targetFrame &&
targetFrame
> != m_frame)| check are impacted by special-casing forms (and these 2 places
are
> the only consumers of |targetFrame| variable).
> 
> Still, I tried various changes described in
>
https://docs.google.com/document/d/1fQIsKlua8aMUQhH7PvKOBlHxJsO9xR2KS0TjTZd-oIE
> and didn't get very far (both - the changes are not a big improvement + I
> couldn't get all the tests to pass).  I am starting to think that even if
there
> are refactoring opportunities here, we should do them in a separate CL (to
keep
> the CL under review simple + to avoid the risk of it getting reverted because
it
> bundles in some extra, opportunistic changes).
> 
> FWIW, going through the exercise of breaking down various conditions helped me
> catch one issue (see
>
https://docs.google.com/document/d/1fQIsKlua8aMUQhH7PvKOBlHxJsO9xR2KS0TjTZd-o...)
> and fix it in the latest patchset.
> 
> PS. In the CL, I did end up moving things around, so that variables are
> introduced right next to the place where they are needed.  I guess this is not
> quite the kind of cleanup you were hoping for... :-/

Yeah, sorry for all the form-related frame targeting workarounds. I tried to
remove them a few years ago, but I broke salesforce in a fairly major
way...right before they were planning on show off their new features in chrome
in the keynote of their big conference. Many people had a very stressful few
days as a result, and I got scared of trying to resolve it :)

[dcheng, 2016-10-18 23:42:00]

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:972: static bool
canIgnoreTargetFrame(NavigationPolicy policy) {
On 2016/10/18 23:26:45, Łukasz Anforowicz wrote:
> On 2016/10/18 23:18:34, dcheng wrote:
> > Minor nit: shouldIgnoreTargetFrame() seems more consistent with the local
> naming
> > convention.
> 
> Thanks for pointing this out.  I think this function answers "can ignore"
rather
> than "should ignore" question - ignoring is optional for local frames (the
> behavior will be the same regardless of which frame we go through);  ignoring
is
> mandatory (i.e. a "should") only if the target is a remote frame (which
doesn't
> handle window dispositions other than CURRENT_TAB).

Shrug, I don't think can or should imply mandatory-ness.

And in any case, we don't differentiate (here) based on whether target frame is
local or remote, AFAICT.

[Łukasz Anforowicz, 2016-10-19 17:05:02]

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

[Łukasz Anforowicz, 2016-10-19 17:05:03]

Alex, can you take a look please?

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2410303006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:972: static bool
canIgnoreTargetFrame(NavigationPolicy policy) {
On 2016/10/18 23:42:00, dcheng wrote:
> On 2016/10/18 23:26:45, Łukasz Anforowicz wrote:
> > On 2016/10/18 23:18:34, dcheng wrote:
> > > Minor nit: shouldIgnoreTargetFrame() seems more consistent with the local
> > naming
> > > convention.
> > 
> > Thanks for pointing this out.  I think this function answers "can ignore"
> rather
> > than "should ignore" question - ignoring is optional for local frames (the
> > behavior will be the same regardless of which frame we go through); 
ignoring
> is
> > mandatory (i.e. a "should") only if the target is a remote frame (which
> doesn't
> > handle window dispositions other than CURRENT_TAB).
> 
> Shrug, I don't think can or should imply mandatory-ness.

Well, there is a difference in how strongly compliance is requested via "can" vs
"should", right?
 
> And in any case, we don't differentiate (here) based on whether target frame
is
> local or remote, AFAICT.

Ack.

FWIW, I've reversed the condition being checked here into something that looks
good (to me) with a "should" - instead of checking if
!canIgnoreTargetFrame(policy) now I am checking
shouldNavigateTargetFrame(policy).  Maybe it is indeed cleaner this way :-)

[Łukasz Anforowicz, 2016-10-19 17:05:06]

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

[commit-bot: I haz the power, 2016-10-19 17:05:41]

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-10-19 19:03:12]

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

[commit-bot: I haz the power, 2016-10-19 19:03:13]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-10-19 23:48:30]

LGTM

https://codereview.chromium.org/2410303006/diff/120001/chrome/test/data/frame...
File chrome/test/data/frame_tree/anchor_targeting_remote_frame.html (right):

https://codereview.chromium.org/2410303006/diff/120001/chrome/test/data/frame...
chrome/test/data/frame_tree/anchor_targeting_remote_frame.html:20:
id="test-anchor">Test link to click while holding shift key</a>
nit: s/shift/ctrl/, since that's what the test does?

[Łukasz Anforowicz, 2016-10-19 23:52:01]

Thanks for reviewing everyone.  I'll push to CQ tomorrow.

https://codereview.chromium.org/2410303006/diff/120001/chrome/test/data/frame...
File chrome/test/data/frame_tree/anchor_targeting_remote_frame.html (right):

https://codereview.chromium.org/2410303006/diff/120001/chrome/test/data/frame...
chrome/test/data/frame_tree/anchor_targeting_remote_frame.html:20:
id="test-anchor">Test link to click while holding shift key</a>
On 2016/10/19 23:48:30, alexmos wrote:
> nit: s/shift/ctrl/, since that's what the test does?

Done.

[Łukasz Anforowicz, 2016-10-20 14:17:22]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-20 14:17:22]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2410303006/#ps140001
(title: "s/shift/ctrl/ in a "comment" in the test html file.")

[commit-bot: I haz the power, 2016-10-20 14:17:41]

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-10-20 15:44:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-21 13:18:32]

Description was changed from

==========
Avoiding going through RenderFrameProxy when targeting a *new* frame.

BUG=647772
==========

to

==========
Avoiding going through RenderFrameProxy when targeting a *new* frame.

BUG=647772

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

[commit-bot: I haz the power, 2016-10-21 13:18:33]

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