OOPIFs: Fixing submitting forms targeting a remote frame.

third_party/WebKit/Source/core/frame/RemoteFrame.cpp

Index: third_party/WebKit/Source/core/frame/RemoteFrame.cpp
diff --git a/third_party/WebKit/Source/core/frame/RemoteFrame.cpp b/third_party/WebKit/Source/core/frame/RemoteFrame.cpp
index ad017062095a9ff0fbbaa8112b4aeccd764d44ef..3f3c7cf10688c4aef63b4516222ef79473289c82 100644
--- a/third_party/WebKit/Source/core/frame/RemoteFrame.cpp
+++ b/third_party/WebKit/Source/core/frame/RemoteFrame.cpp
@@ -13,10 +13,12 @@
 #include "core/frame/RemoteFrameView.h"
 #include "core/html/HTMLFrameOwnerElement.h"
 #include "core/loader/FrameLoadRequest.h"
+#include "core/loader/FrameLoader.h"
 #include "core/paint/PaintLayer.h"
 #include "platform/PluginScriptForbiddenScope.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/graphics/GraphicsLayer.h"
+#include "platform/network/ResourceRequest.h"
 #include "platform/weborigin/SecurityPolicy.h"
 #include "public/platform/WebLayer.h"
 
@@ -66,18 +68,23 @@ WindowProxy* RemoteFrame::windowProxy(DOMWrapperWorld& world)
 
 void RemoteFrame::navigate(Document& originDocument, const KURL& url, bool replaceCurrentItem, UserGestureStatus userGestureStatus)
 {
-    // The process where this frame actually lives won't have sufficient information to determine
-    // correct referrer, since it won't have access to the originDocument. Set it now.
-    ResourceRequest request(url);
-    request.setHTTPReferrer(SecurityPolicy::generateReferrer(originDocument.getReferrerPolicy(), url, originDocument.outgoingReferrer()));

[Łukasz Anforowicz, 2016/06/08 20:32:43]

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

-    request.setHasUserGesture(userGestureStatus == UserGestureStatus::Active);
-    client()->navigate(request, replaceCurrentItem);
+    FrameLoadRequest frameRequest(&originDocument);

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

frameRequest(&originDocument, url) is fine here instead of calling setURL()
later, KURL->ResourceRequest is a permitted implicit conversion.

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

Done.

+    frameRequest.setReplacesCurrentItem(replaceCurrentItem);
+    frameRequest.resourceRequest().setURL(url);
+    frameRequest.resourceRequest().setHasUserGesture(userGestureStatus == UserGestureStatus::Active);

[Łukasz Anforowicz, 2016/06/08 20:32:43]

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

+    navigate(frameRequest);
 }
 
 void RemoteFrame::navigate(const FrameLoadRequest& passedRequest)
 {
-    UserGestureStatus gesture = UserGestureIndicator::processingUserGesture() ? UserGestureStatus::Active : UserGestureStatus::None;
-    navigate(*passedRequest.originDocument(), passedRequest.resourceRequest().url(), passedRequest.replacesCurrentItem(), gesture);
+    FrameLoadRequest frameRequest(passedRequest);
+
+    // The process where this frame actually lives won't have sufficient information to determine
+    // correct referrer, since it won't have access to the originDocument. Set it now.
+    FrameLoader::setReferrerForFrameRequest(frameRequest);
+
+    frameRequest.resourceRequest().setHasUserGesture(UserGestureIndicator::processingUserGesture());
+    client()->navigate(frameRequest.resourceRequest(), frameRequest.replacesCurrentItem());
 }

[Łukasz Anforowicz, 2016/06/08 20:32:43]

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

 
 void RemoteFrame::reload(FrameLoadType frameLoadType, ClientRedirectPolicy clientRedirectPolicy)