OOPIFs: Fixing submitting forms targeting a remote frame.

OOPIFs: Fixing submitting forms targeting a remote frame.

The fix makes form submissions targeting a remote frame go through
RenderFrameProxy::navigate which forwards them to the browser via
FrameHostMsg_OpenURL IPC message.  Right now this IPC only supports
HTTP GET method (support for POST method is being fixed in parallel
- see https://crbug.com/344348).

BUG=585284
Committed: https://crrev.com/fa9b547938cddc922a7a51e73d5416bc4520f274
Cr-Commit-Position: refs/heads/master@{#399524}

[Łukasz Anforowicz, 2016-06-03 22:41:44]

Description was changed from

==========
OOPIFs: Fixing submitting forms targeting a remote frame.

BUG=585284
==========

to

==========
OOPIFs: Fixing submitting forms targeting a remote frame.

The fix makes form submissions targeting a remote frame go through
RenderFrameProxy::navigate which forwards them to the browser via
FrameHostMsg_OpenURL IPC message.  Right now this IPC only supports
HTTP GET method (support for POST method is being fixed in parallel
- see https://crbug.com/344348).

BUG=585284
==========

[Łukasz Anforowicz, 2016-06-04 00:03:39]

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

[Łukasz Anforowicz, 2016-06-04 00:03:40]

Nate, could you please take a look?

The current CL seems to fix submissions of forms targeting remote
frames (as long as they go through HTTP GET method).  At the same time
I am not very familiar with the code I am changing, so maybe there is
a better approach.  In particular, there are 2 FIXMEs in the old code
and my changes only touch 1 of the FIXMEs (and I don't know if the other
FIXME should worry me / what the repro steps for hitting it would be):

- core/html/HTMLFormElement.cpp:419:
  HTMLFormElement::scheduleFormSubmission:
  FIXME fixed by this CL

- core/page/CreateWindow.cpp:216:
  createWindowForRequest:
  has TODO(japhet) that this CL doesn't touch


Some more notes below:

*) Why not go through RemoteFrame::navigate(const FrameLoadRequest&)
   [instead of through
    RemoteFrameClient::navigate(const ResourceRequest&, ...))]

    - Currently RemoteFrame::navigate(const FrameLoadRequest&) creates
      a brand new request, and only copies over url (+ sets referrer
      and ugi).  In particular the HTTP method, body and contentType
      are not copied.

*) Why do the work in HTMLFormElement::scheduleFormSubmission
   [instead of in createWindowForRequest, where the other FIXME is]?

   - Policy trouble (this is what made me stop and reconsider just
     going with the current fix):

      - When the form is submitted via javascript (i.e. form.submit()
        call), then there will be no event in
        FrameLoadRequest::trigerringEvent().

      - This will mean that navigationPolicyForRequest will always
        return NavigationPolicyCurrentTab.

      - This will mean that createWindowForRequest will always call
        createWindowHelper with
        policy=NavigationPolicyNewForegroundTab.

      - This will mean that reuseExistingWindow will never find any
        frame (it exits early unless policy==NavigationPolicyIgnore).

   - No known repro that would require remote frames support in
     createWindowForRequest

        - In createWindowForRequest only previously existing frames
          can be remote frames.  Targeting a frame at a new window
          will work via a local frame (we might hit a process transfer
          *later*).

        - I don’t understand in what scenario createWindowForRequest
          can encounter a remote frame.  Having repro steps for this
          would be great.

   - Cleared target

        - Currently HTMLFormElement::scheduleFormSubmission calls
          submission->clearTarget().

        - The above means that FrameLoader::load always targets the
          current frame.

   - Navigation through form-owning-frame vs through target-frame

        - Currently (for local frames at least)
          HTMLFormElement::scheduleFormSubmission goes through target
          frame’s navigation scheduler.

        - If we wanted to reuse the form navigation scheduling logic
          for remote frames, we would need to go through
          form-owning-frame’s navigation scheduler.  I am not sure if
          this is okay.

*) Current fix seems okay behavior-wise

   - We seem to take care of all the things that the local frame
     scenario:

        - User gesture indicator is taken care of by
          RenderFrameProxy::navigate

        - We set the referrer in the new code in
          HTMLFormElement::scheduleFormSubmission

        - I assume that there is no need to copy any policies (i.e.
          referrer policy) for an already *existing* frame.

        - Local frame’s navigation will be scheduled and happen later,
          asynchronously.  Remote frame’s navigation is also
          asynchronous (and it’s effects will only be felt after
          unwinding the current stack / after processing new IPC
          messages).

   - Differences seem okay:

        - The remote frame scenario always says “create new history
          entry”.  In local frame’s case this is decided via
          toLocalFrame(parentFrame)->loader().allAncestorsAreComplete()

        - I don’t think FrameLoadRequest::setTrigerringEvent and/or
          setForm are important

*) I am unsure if extra things should be done to prevent XSS

   - Location::setLocation checks
     m_frame->domWindow()->isInsecureScriptAccess before calling
     (Remote)Frame::navigate.


PS. I am sending out the CR without checking if the bots are green, but they
were green in patchset 3, so hopefully they should stay green (if they don't I
likely need to merge / rebase FlagExpectations/site-per-process changes).

[Nate Chapin, 2016-06-07 21:35:51]

Sorry for the delay...

This seems reasonable, just a few code formatting nits.

It's probably worth looking at RemoteFrame::navigate to see if we can refactor
it to not through away ResourceRequest properties besides the url. If it's not
quick and easy, feel free to punt on it for now.

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FormSubmission.cpp (right):

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FormSubmission.cpp:273: void
FormSubmission::populateFrameLoadRequest(FrameLoadRequest& frameRequest)
I'm not sure this is worth keeping anymore. Maybe just have the sole caller get
the ResourceRequest from FormSubmission and setFrameName() directly?

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FormSubmission.cpp:281: void
FormSubmission::populateResourceRequest(ResourceRequest& resourceRequest)
Maybe have this return a ResourceRequest instead of populating an existing one?

[Łukasz Anforowicz, 2016-06-08 20:32:44]

Thanks Nate.  Could you please take another look?

On 2016/06/07 21:35:51, Nate Chapin wrote:
> It's probably worth looking at RemoteFrame::navigate to see if we can refactor
> it to not through away ResourceRequest properties besides the url. If it's not
> quick and easy, feel free to punt on it for now.

I've talked with Nasko (who initially added
RemoteFrame::navigate(FrameLoadRequest&) in https://crrev.com/1152053006) and it
does seem that refactoring RemoteFrame::navigate overloads is a good idea - I've
done this in the latest patchset.  This probably means having to restart review
from scratch (sorry) but I think this is worth it (if only to avoid ad-hoc and
out-of-place setting of referrer inside HTMLFormElement.cpp).

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FormSubmission.cpp (right):

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FormSubmission.cpp:273: void
FormSubmission::populateFrameLoadRequest(FrameLoadRequest& frameRequest)
On 2016/06/07 21:35:51, Nate Chapin wrote:
> I'm not sure this is worth keeping anymore. Maybe just have the sole caller
get
> the ResourceRequest from FormSubmission and setFrameName() directly?

Actually, I decided to go through RemoteFrame::navigate (which takes a
FrameLoadRequest& rather than ResourceRequest&) so let me keep the whole
populateFrameLoadRequest and avoid introducing a separate
populateResourceRequest.

https://codereview.chromium.org/1999573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FormSubmission.cpp:281: void
FormSubmission::populateResourceRequest(ResourceRequest& resourceRequest)
On 2016/06/07 21:35:51, Nate Chapin wrote:
> Maybe have this return a ResourceRequest instead of populating an existing
one?

Good idea.  Let me change
    void FormSubmission::populateFrameLoadRequest(FrameLoadRequest&)
into
    FrameLoadRequest FormSubmission::createFrameLoadRequest(Document*
originDocument)

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (left):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:72:
request.setHTTPReferrer(SecurityPolicy::generateReferrer(originDocument.getReferrerPolicy(),
url, originDocument.outgoingReferrer()));
In theory, I could have just moved this setHTTPReferrer call to the other
::navigate overload, but instead I decided to replace it with a call to
FrameLoader::setReferrerForFrameRequest.  This is because
FrameLoader::setReferrerForFrameRequest seems more complete + calling it seems
good from code deduplication / DRY perspective.  For example
FrameLoader::setReferrerForFrameRequest also calls
ResourceRequest::addHTTPOriginIfNeeded + consults
FrameLoadRequest::getShouldSendReferrer (which defaults to MaybeSendReferrer,
but still...).

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:81: }
The changes here make navigate(const FrameLoadRequest&) the main overload that
everything goes through (i.e. the overload that actually calls out into
RemoteFrameClient::navigate(ResourceRequest&, bool).

I've talked to Nasko and he says that it is accidental that the old code of
navigate(const FrameLoadRequest&) used to copy only a few pieces from the old
request (i.e. there were no security reasons for doing this / this was mostly
done for code reuse).  This is great - in this CL we actually need to forward
the whole request (i.e. we care about keeping the HTTP body).

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:74:
frameRequest.resourceRequest().setHasUserGesture(userGestureStatus ==
UserGestureStatus::Active);
Calling setHasUserGesture twice (in both overloads of ::navigate) seems ok -
once it this method flips |m_hasUserGesture| to true, it will stay set to true
going forward:

void ResourceRequest::setHasUserGesture(bool hasUserGesture)
{
    m_hasUserGesture |= hasUserGesture;
}


Calling setHasUserGesture twice (in both overloads of ::navigate) is needed to:

1) preserve behavior of ::navigate(Document&, KURL&, ...) overload, which needs
to use the |userGestureStatus| argument.

2) make sure we also set user gesture indicator when going only through
::navigate(FrameLoadRequest&) overload (something that wasn't possible before
this CL).


OTOH, I am not sure how important this is - after all, this ResourceRequest is
only ever passed to RemoteFrameProxy::navigate which doesn't look at the
contents of the user gesture indicator *inside* ResourceRequest / WebURLRequest,
but instead directly inspects WebUserGestureIndicator::isProcessingUserGesture()
and unconditionally calls WebUserGestureIndicator::consumeUserGesture().

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FormSubmission.cpp (right):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FormSubmission.cpp:294:
frameRequest.setForm(m_form);
These 2 statements have been moved here from
ScheduledFormSubmission::fire.

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FormSubmission.h (left):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FormSubmission.h:105: Event* event() const
{ return m_event.get(); }
This accessor is no longer needed after folding 2 statements from
ScheduledFormSubmission::fire into FormSubmission::createFrameLoadRequest.

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FormSubmission.h (right):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FormSubmission.h:34: #include
"core/loader/FrameLoadRequest.h"
Needed for createFrameLoadRequest, but introduces an include-loop that needs to
be broken by a fwd-decl in FrameLoadRequest.h

[Nate Chapin, 2016-06-09 23:40:43]

LGTM with a small formatting nit

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:71: FrameLoadRequest
frameRequest(&originDocument);
frameRequest(&originDocument, url) is fine here instead of calling setURL()
later, KURL->ResourceRequest is a permitted implicit conversion.

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

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

[commit-bot: I haz the power, 2016-06-10 16:16:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999573003/140001

[Łukasz Anforowicz, 2016-06-10 16:17:25]

Thanks for reviewing.

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1999573003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:71: FrameLoadRequest
frameRequest(&originDocument);
On 2016/06/09 23:40:43, Nate Chapin wrote:
> frameRequest(&originDocument, url) is fine here instead of calling setURL()
> later, KURL->ResourceRequest is a permitted implicit conversion.

Done.

[commit-bot: I haz the power, 2016-06-10 16:35:55]

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

[commit-bot: I haz the power, 2016-06-10 16:35:56]

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

[Łukasz Anforowicz, 2016-06-10 16:41:23]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-06-10 16:41:24]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1999573003/#ps140001
(title: "Replaced setURL call with calling a different FrameLoadRequest
constructor.")

[commit-bot: I haz the power, 2016-06-10 16:41:50]

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

[commit-bot: I haz the power, 2016-06-10 16:48:55]

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

[commit-bot: I haz the power, 2016-06-10 16:48:56]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2016-06-10 18:09:53]

+ikilpatrick@ as an FYI for the IWYU fix in
FilteredComputedStylePropertyMapTest.cpp

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

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-06-10 18:11:58]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1999573003/#ps180001
(title: "Fixing IWYU problem exposed by changes in FrameLoadRequest.h")

[commit-bot: I haz the power, 2016-06-10 18:12:24]

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

[commit-bot: I haz the power, 2016-06-10 21:03:03]

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

[commit-bot: I haz the power, 2016-06-10 21:03:05]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Łukasz Anforowicz, 2016-06-10 21:33:12]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-10 21:33:54]

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

[commit-bot: I haz the power, 2016-06-11 00:23:37]

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

[commit-bot: I haz the power, 2016-06-11 00:23:38]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2016-06-12 05:50:28]

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

[dcheng, 2016-06-12 05:50:29]

https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFormElement.cpp:429:
toRemoteFrame(targetFrame)->navigate(frameLoadRequest);
Crazy idea: is it possible to just call Frame::navigate() for both paths here?
We can just always do this in a timer so we still get the scheduled effect.

[Łukasz Anforowicz, 2016-06-13 17:13:43]

Daniel, I think I'd still want to try to land as-is - please see my longer reply
below.

Nate, I assume that landing as-is is still okay with you?  Any feedback on
uniform handling of remote and local frames (for form submission from this CL,
but also potentially more broadly - for more scheduled navigation flavours)?

https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFormElement.cpp:429:
toRemoteFrame(targetFrame)->navigate(frameLoadRequest);
On 2016/06/12 05:50:29, dcheng wrote:
> Crazy idea: is it possible to just call Frame::navigate() for both paths here?
> We can just always do this in a timer so we still get the scheduled effect.

Hmmm.  This does sound like something we should at least seriously attempt.  At
the same time I'd prefer to keep the current CL as-is - the alternative approach
has higher risk of changing behavior for local frames (not really touched in the
current CL) and so containing the changes in a separate CL seems desirable (to
enable evaluating the impact in separation + to enable reverting in separation).
 I also have hidden motivation for keeping the current CL as-is: 1) this will
help me rebase and land the OpenURL CLs and 2) I want to minimize the risk the
POST work won't land before my vacation starts next week... :-/

I see the following aspects of behavior we would need to ensure we are
preserving (if replacing navigationScheduler for the local case with an ad-hoc
timer + virtual Frame::navigate call):

- NavigationScheduler::mustReplaceCurrentItem(LocalFrame* targetFrame) is
currently private and therefore only callable via the current path.  We would
need to figure out how to duplicate the behavior outside of the current code
path without duplicating the code.  OTOH, I do note that calculation of
|replaceCurrentItem| for remote frames seems sketchy in the current CL (well,
there is no calculation - we depend on replaceCurrentItem being set by default
to false by the constructors of FrameLoadRequest).

- It is not immediately clear how to preserve maybeLogScheduledNavigationClobber
outside of navigationScheduler path.

- I don't quite understand the significance of instantiating
OwnPtr<UserGestureIndicator> gestureIndicator = createUserGestureIndicator(); in
ScheduledFormSubmission::fire (as opposed to just calling static methods of
UserGestureIndicator, without constructing an instance).

Additionally - we probably should audit other scheduled navigations in
core/loader/NavigationScheduler.cpp and see if they should also be potentially
replaced with a timer-based call to Frame::navigate (i.e. if these navigations
can target a remote frame).

FWIW, I've added a TODO here...

[Łukasz Anforowicz, 2016-06-13 17:14:05]

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

[commit-bot: I haz the power, 2016-06-13 17:14:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999573003/200001

[commit-bot: I haz the power, 2016-06-13 19:14:50]

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

[commit-bot: I haz the power, 2016-06-13 19:14:51]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-06-13 20:23:44]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-06-13 20:23:44]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1999573003/#ps200001
(title: "Added a TODO for investigating if remote and local frames can be
handled uniformly.")

[commit-bot: I haz the power, 2016-06-13 20:24:25]

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

[dcheng, 2016-06-13 20:26:56]

On 2016/06/13 17:13:43, Łukasz Anforowicz wrote:
> Daniel, I think I'd still want to try to land as-is - please see my longer
reply
> below.
> 
> Nate, I assume that landing as-is is still okay with you?  Any feedback on
> uniform handling of remote and local frames (for form submission from this CL,
> but also potentially more broadly - for more scheduled navigation flavours)?
> 
>
https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right):
> 
>
https://codereview.chromium.org/1999573003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLFormElement.cpp:429:
> toRemoteFrame(targetFrame)->navigate(frameLoadRequest);
> On 2016/06/12 05:50:29, dcheng wrote:
> > Crazy idea: is it possible to just call Frame::navigate() for both paths
here?
> > We can just always do this in a timer so we still get the scheduled effect.
> 
> Hmmm.  This does sound like something we should at least seriously attempt. 
At
> the same time I'd prefer to keep the current CL as-is - the alternative
approach
> has higher risk of changing behavior for local frames (not really touched in
the
> current CL) and so containing the changes in a separate CL seems desirable (to
> enable evaluating the impact in separation + to enable reverting in
separation).
>  I also have hidden motivation for keeping the current CL as-is: 1) this will
> help me rebase and land the OpenURL CLs and 2) I want to minimize the risk the
> POST work won't land before my vacation starts next week... :-/
> 
> I see the following aspects of behavior we would need to ensure we are
> preserving (if replacing navigationScheduler for the local case with an ad-hoc
> timer + virtual Frame::navigate call):
> 
> - NavigationScheduler::mustReplaceCurrentItem(LocalFrame* targetFrame) is
> currently private and therefore only callable via the current path.  We would
> need to figure out how to duplicate the behavior outside of the current code
> path without duplicating the code.  OTOH, I do note that calculation of
> |replaceCurrentItem| for remote frames seems sketchy in the current CL (well,
> there is no calculation - we depend on replaceCurrentItem being set by default
> to false by the constructors of FrameLoadRequest).
> 
> - It is not immediately clear how to preserve
maybeLogScheduledNavigationClobber
> outside of navigationScheduler path.
> 
> - I don't quite understand the significance of instantiating
> OwnPtr<UserGestureIndicator> gestureIndicator = createUserGestureIndicator();
in
> ScheduledFormSubmission::fire (as opposed to just calling static methods of
> UserGestureIndicator, without constructing an instance).
> 
> Additionally - we probably should audit other scheduled navigations in
> core/loader/NavigationScheduler.cpp and see if they should also be potentially
> replaced with a timer-based call to Frame::navigate (i.e. if these navigations
> can target a remote frame).
> 
> FWIW, I've added a TODO here...

OK, I'm fine with landing as-is: this is strictly better than what we have. But
we should try to move it into a virtual method in a followup: having to do
manual dispatch like this is usually a sign that we should clean up some code =)

[commit-bot: I haz the power, 2016-06-13 20:29:37]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-13 20:30:15]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 20:30:59]

Description was changed from

==========
OOPIFs: Fixing submitting forms targeting a remote frame.

The fix makes form submissions targeting a remote frame go through
RenderFrameProxy::navigate which forwards them to the browser via
FrameHostMsg_OpenURL IPC message.  Right now this IPC only supports
HTTP GET method (support for POST method is being fixed in parallel
- see https://crbug.com/344348).

BUG=585284
==========

to

==========
OOPIFs: Fixing submitting forms targeting a remote frame.

The fix makes form submissions targeting a remote frame go through
RenderFrameProxy::navigate which forwards them to the browser via
FrameHostMsg_OpenURL IPC message.  Right now this IPC only supports
HTTP GET method (support for POST method is being fixed in parallel
- see https://crbug.com/344348).

BUG=585284

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

[commit-bot: I haz the power, 2016-06-13 20:31:01]

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