PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate output from WebFrameClie…

PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate output from WebFrameClient::willSendRequest.

With PlzNavigate, the willSendRequest function is called in RenderFrameImpl::BeginNavigation() and again during OnCommitNavigation()
when the request is loaded by blink. One option which was considered was to call willSendRequest() in OnCommitNavigation(). However the
request may be blocked and hence that call may never happen.

To workaround this I added a flag to track renderer initiated navigations to the RequestExtraData structure. We use this flag to determine
whether the WebFrameTestClient::willSendRequest() function needs to early return or not. The assumption is that the logging would already
be done when the navigation was initiated for renderer initiated navigations. No need to log when it is committed.

The other changes are to add functionality to the WebTestDelegate interface for the runner to query this flag.

There is one more test marked under the same bug http/tests/loading/redirect-methods.html. Does not appear to be the same issue. Will debug and log
a new bug if required.

BUG=625765
Review-Url: https://codereview.chromium.org/2652123002
Cr-Commit-Position: refs/heads/master@{#446462}
Committed: https://chromium.googlesource.com/chromium/src/+/fce540a08089df75d18cd4d719bec9ca03e88ac5

[ananta, 2017-01-24 23:37:26]

ananta@chromium.org changed reviewers:
+ jam@chromium.org, japhet@chromium.org

[ananta, 2017-01-24 23:37:36]

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

[commit-bot: I haz the power, 2017-01-24 23:38:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-01-24 23:52:23]

I don't really like having this bit here for test harness use only. Could we
skip the 2nd willSendRequest() call entirely instead?

[ananta, 2017-01-24 23:56:27]

On 2017/01/24 23:52:23, Nate Chapin wrote:
> I don't really like having this bit here for test harness use only. Could we
> skip the 2nd willSendRequest() call entirely instead?

The second call happens when the request is loaded in blink from the
FrameLoaderClient callstack.
We don't have enough context there to decide to skip it?

[Nate Chapin, 2017-01-25 00:05:06]

On 2017/01/24 23:56:27, ananta wrote:
> On 2017/01/24 23:52:23, Nate Chapin wrote:
> > I don't really like having this bit here for test harness use only. Could we
> > skip the 2nd willSendRequest() call entirely instead?
> 
> The second call happens when the request is loaded in blink from the
> FrameLoaderClient callstack.
> We don't have enough context there to decide to skip it?

If we can inspect the bit at WebFrameTestClient::willSendRequest(), we can
presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does that
break anything?

[ananta, 2017-01-25 00:24:29]

On 2017/01/25 00:05:06, Nate Chapin wrote:
> On 2017/01/24 23:56:27, ananta wrote:
> > On 2017/01/24 23:52:23, Nate Chapin wrote:
> > > I don't really like having this bit here for test harness use only. Could
we
> > > skip the 2nd willSendRequest() call entirely instead?
> > 
> > The second call happens when the request is loaded in blink from the
> > FrameLoaderClient callstack.
> > We don't have enough context there to decide to skip it?
> 
> If we can inspect the bit at WebFrameTestClient::willSendRequest(), we can
> presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does that
> break anything?

RenderFrameImpl::willSendRequest gets called in layout tests from here.
https://cs.chromium.org/chromium/src/components/test_runner/web_frame_test_pr...

The Proxy is created here.
https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?q=Web...

We cannot skip the call given how the code is written currently.

[Nate Chapin, 2017-01-25 00:32:27]

On 2017/01/25 00:24:29, ananta wrote:
> On 2017/01/25 00:05:06, Nate Chapin wrote:
> > On 2017/01/24 23:56:27, ananta wrote:
> > > On 2017/01/24 23:52:23, Nate Chapin wrote:
> > > > I don't really like having this bit here for test harness use only.
Could
> we
> > > > skip the 2nd willSendRequest() call entirely instead?
> > > 
> > > The second call happens when the request is loaded in blink from the
> > > FrameLoaderClient callstack.
> > > We don't have enough context there to decide to skip it?
> > 
> > If we can inspect the bit at WebFrameTestClient::willSendRequest(), we can
> > presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does that
> > break anything?
> 
> RenderFrameImpl::willSendRequest gets called in layout tests from here.
>
https://cs.chromium.org/chromium/src/components/test_runner/web_frame_test_pr...
> 
> The Proxy is created here.
>
https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?q=Web...
> 
> We cannot skip the call given how the code is written currently.

Maybe "skip" wasn't the right word: if we're early-exiting from
WebFrameTestClient::willSendRequest(), should we early-exit from
RenderFrameImpl::willSendRequest(), too?

[ananta, 2017-01-25 00:36:21]

On 2017/01/25 00:32:27, Nate Chapin wrote:
> On 2017/01/25 00:24:29, ananta wrote:
> > On 2017/01/25 00:05:06, Nate Chapin wrote:
> > > On 2017/01/24 23:56:27, ananta wrote:
> > > > On 2017/01/24 23:52:23, Nate Chapin wrote:
> > > > > I don't really like having this bit here for test harness use only.
> Could
> > we
> > > > > skip the 2nd willSendRequest() call entirely instead?
> > > > 
> > > > The second call happens when the request is loaded in blink from the
> > > > FrameLoaderClient callstack.
> > > > We don't have enough context there to decide to skip it?
> > > 
> > > If we can inspect the bit at WebFrameTestClient::willSendRequest(), we can
> > > presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does
that
> > > break anything?
> > 
> > RenderFrameImpl::willSendRequest gets called in layout tests from here.
> >
>
https://cs.chromium.org/chromium/src/components/test_runner/web_frame_test_pr...
> > 
> > The Proxy is created here.
> >
>
https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?q=Web...
> > 
> > We cannot skip the call given how the code is written currently.
> 
> Maybe "skip" wasn't the right word: if we're early-exiting from
> WebFrameTestClient::willSendRequest(), should we early-exit from
> RenderFrameImpl::willSendRequest(), too?

Will look into that.

Thanks

[ananta, 2017-01-25 00:53:46]

On 2017/01/25 00:36:21, ananta wrote:
> On 2017/01/25 00:32:27, Nate Chapin wrote:
> > On 2017/01/25 00:24:29, ananta wrote:
> > > On 2017/01/25 00:05:06, Nate Chapin wrote:
> > > > On 2017/01/24 23:56:27, ananta wrote:
> > > > > On 2017/01/24 23:52:23, Nate Chapin wrote:
> > > > > > I don't really like having this bit here for test harness use only.
> > Could
> > > we
> > > > > > skip the 2nd willSendRequest() call entirely instead?
> > > > > 
> > > > > The second call happens when the request is loaded in blink from the
> > > > > FrameLoaderClient callstack.
> > > > > We don't have enough context there to decide to skip it?
> > > > 
> > > > If we can inspect the bit at WebFrameTestClient::willSendRequest(), we
can
> > > > presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does
> that
> > > > break anything?
> > > 
> > > RenderFrameImpl::willSendRequest gets called in layout tests from here.
> > >
> >
>
https://cs.chromium.org/chromium/src/components/test_runner/web_frame_test_pr...
> > > 
> > > The Proxy is created here.
> > >
> >
>
https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?q=Web...
> > > 
> > > We cannot skip the call given how the code is written currently.
> > 
> > Maybe "skip" wasn't the right word: if we're early-exiting from
> > WebFrameTestClient::willSendRequest(), should we early-exit from
> > RenderFrameImpl::willSendRequest(), too?
> 
> Will look into that.
> 
> Thanks

The RenderFrameImpl::willSendRequest() function sets a number of properties in
the WebURLRequest instance. I don't think we can skip the function in its
entirety. The part which can possibly be skipped may be the call to
GetContentClient()->renderer()->WillSendRequest()

[commit-bot: I haz the power, 2017-01-25 01:20:51]

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

[commit-bot: I haz the power, 2017-01-25 01:20:52]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-25 05:18:50]

On 2017/01/25 00:53:46, ananta wrote:
> On 2017/01/25 00:36:21, ananta wrote:
> > On 2017/01/25 00:32:27, Nate Chapin wrote:
> > > On 2017/01/25 00:24:29, ananta wrote:
> > > > On 2017/01/25 00:05:06, Nate Chapin wrote:
> > > > > On 2017/01/24 23:56:27, ananta wrote:
> > > > > > On 2017/01/24 23:52:23, Nate Chapin wrote:
> > > > > > > I don't really like having this bit here for test harness use
only.
> > > Could
> > > > we
> > > > > > > skip the 2nd willSendRequest() call entirely instead?
> > > > > > 
> > > > > > The second call happens when the request is loaded in blink from the
> > > > > > FrameLoaderClient callstack.
> > > > > > We don't have enough context there to decide to skip it?
> > > > > 
> > > > > If we can inspect the bit at WebFrameTestClient::willSendRequest(), we
> can
> > > > > presumably inspect it at RenderFrameImpl::willSendRequest(), too? Does
> > that
> > > > > break anything?
> > > > 
> > > > RenderFrameImpl::willSendRequest gets called in layout tests from here.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/test_runner/web_frame_test_pr...
> > > > 
> > > > The Proxy is created here.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/test/layouttest_support.cc?q=Web...
> > > > 
> > > > We cannot skip the call given how the code is written currently.
> > > 
> > > Maybe "skip" wasn't the right word: if we're early-exiting from
> > > WebFrameTestClient::willSendRequest(), should we early-exit from
> > > RenderFrameImpl::willSendRequest(), too?
> > 
> > Will look into that.
> > 
> > Thanks
> 
> The RenderFrameImpl::willSendRequest() function sets a number of properties in
> the WebURLRequest instance. I don't think we can skip the function in its
> entirety. The part which can possibly be skipped may be the call to
> GetContentClient()->renderer()->WillSendRequest()

I don't think we can skip the RenderFrameImpl::willSendRequest() function in
either case. The function sets a bunch
of properties like requester origin, user agent, etc which are referred later
on.

[Nate Chapin, 2017-01-25 18:27:39]

It's a shame that this is necessary, but I agree that there isn't a great
alternative. Maybe when PlzNavigate is permanently enabled, we can drop the bit
and update the test results instead. LGTM.

https://codereview.chromium.org/2652123002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2652123002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebURLRequest.h:339: BLINK_PLATFORM_EXPORT
bool getNavigationInitiatedByRenderer() const;
Please note here (or elsewhere if you think there's a better place) that this
bit is only read in tests.

[jam, 2017-01-25 18:36:32]

Since this is only used by content/, instead of modifying webkit/ can we store
this bit on RequestExtraData instead? You can add a content/public/test api to
read the bit from RequestExtraData which is internal.

[ananta, 2017-01-25 23:04:55]

Description was changed from

==========
PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate
output from WebFrameClient::willSendRequest.

With PlzNavigate, the willSendRequest function is called in
RenderFrameImpl::BeginNavigation() and again during OnCommitNavigation()
when the request is loaded by blink. One option which was considered was to call
willSendRequest() in OnCommitNavigation(). However the
request may be blocked and hence that call may never happen.

To workaround this, I added a flag m_navigationInitiatedByRenderer to the
ResourceRequest class. This is set and retrieved via accessor methods
setNavigationInitiatedByRenderer() and getNavigationInitiatedByRenderer(). We
use the nav_entry_id field in the RequestNavigationParams structure
to determine whether a navigation was initiated by the browser or renderer and
set this accordingly.

There is one more test marked under the same bug
http/tests/loading/redirect-methods.html. Does not appear to be the same issue.
Will debug and log
a new bug if required.

BUG=625765
==========

to

==========
PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate
output from WebFrameClient::willSendRequest.

With PlzNavigate, the willSendRequest function is called in
RenderFrameImpl::BeginNavigation() and again during OnCommitNavigation()
when the request is loaded by blink. One option which was considered was to call
willSendRequest() in OnCommitNavigation(). However the
request may be blocked and hence that call may never happen.

To workaround this I added a flag to track renderer initiated navigations to the
RequestExtraData structure. We use this flag to determine
whether the WebFrameTestClient::willSendRequest() function needs to early return
or not. The assumption is that the logging would already
be done when the navigation was initiated for renderer initiated navigations. No
need to log when it is committed.

The other changes are to add functionality to the WebTestDelegate interface for
the runner to query this flag.

There is one more test marked under the same bug
http/tests/loading/redirect-methods.html. Does not appear to be the same issue.
Will debug and log
a new bug if required.

BUG=625765
==========

[ananta, 2017-01-25 23:05:08]

On 2017/01/25 18:36:32, jam wrote:
> Since this is only used by content/, instead of modifying webkit/ can we store
> this bit on RequestExtraData instead? You can add a content/public/test api to
> read the bit from RequestExtraData which is internal.

Thanks. Done.

[ananta, 2017-01-25 23:06:03]

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

[commit-bot: I haz the power, 2017-01-25 23:07:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-25 23:59:05]

lgtm

https://codereview.chromium.org/2652123002/diff/40001/content/shell/renderer/...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2652123002/diff/40001/content/shell/renderer/...
content/shell/renderer/layout_test/blink_test_runner.cc:752: return
content::IsNavigationInitiatedByRenderer(request);
nit: no content::

[commit-bot: I haz the power, 2017-01-26 01:04:28]

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

[commit-bot: I haz the power, 2017-01-26 01:04:29]

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

[ananta, 2017-01-26 03:23:44]

https://codereview.chromium.org/2652123002/diff/40001/content/shell/renderer/...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2652123002/diff/40001/content/shell/renderer/...
content/shell/renderer/layout_test/blink_test_runner.cc:752: return
content::IsNavigationInitiatedByRenderer(request);
On 2017/01/25 23:59:05, jam wrote:
> nit: no content::

Done.

[ananta, 2017-01-26 03:23:53]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-26 03:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2652123002/#ps60001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-01-26 03:24:53]

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, 2017-01-26 03:38:04]

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

[commit-bot: I haz the power, 2017-01-26 03:38:05]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-01-26 16:34:57]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-26 16:34:57]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2652123002/#ps80001
(title: "Fix redness")

[commit-bot: I haz the power, 2017-01-26 16:35:38]

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, 2017-01-26 18:01:20]

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

[commit-bot: I haz the power, 2017-01-26 18:01:22]

Failed to apply patch for content/test/layouttest_support.cc:
While running git apply --index -p1;
  error: patch failed: content/test/layouttest_support.cc:23
  error: content/test/layouttest_support.cc: patch does not apply

Patch:       content/test/layouttest_support.cc
Index: content/test/layouttest_support.cc
diff --git a/content/test/layouttest_support.cc
b/content/test/layouttest_support.cc
index
117f9bd066ab3b8484fedb3ca73e727e92d9d6b3..85e3439cc6c33a6c3d33c9ddd8235b1cd342c331
100644
--- a/content/test/layouttest_support.cc
+++ b/content/test/layouttest_support.cc
@@ -23,6 +23,7 @@
 #include "content/browser/bluetooth/bluetooth_device_chooser_controller.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
+#include "content/child/request_extra_data.h"
 #include "content/common/renderer.mojom.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/common/page_state.h"
@@ -608,4 +609,10 @@ void ForceTextInputStateUpdateForRenderFrame(RenderFrame*
frame) {
   }
 }
 
+bool IsNavigationInitiatedByRenderer(const blink::WebURLRequest& request) {
+  RequestExtraData* extra_data = static_cast<RequestExtraData*>(
+      request.getExtraData());
+  return extra_data && extra_data->navigation_initiated_by_renderer();
+}
+
 }  // namespace content

[ananta, 2017-01-26 20:16:04]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-26 20:16:04]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2652123002/#ps100001
(title: "Fix redness")

[commit-bot: I haz the power, 2017-01-26 20:16:22]

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, 2017-01-26 21:53:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485461764239310,
"parent_rev": "1eb5fbf8b4dc3c28994b3ea0d27b724ab9f26f78", "commit_rev":
"fce540a08089df75d18cd4d719bec9ca03e88ac5"}

[commit-bot: I haz the power, 2017-01-26 21:53:45]

Description was changed from

==========
PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate
output from WebFrameClient::willSendRequest.

With PlzNavigate, the willSendRequest function is called in
RenderFrameImpl::BeginNavigation() and again during OnCommitNavigation()
when the request is loaded by blink. One option which was considered was to call
willSendRequest() in OnCommitNavigation(). However the
request may be blocked and hence that call may never happen.

To workaround this I added a flag to track renderer initiated navigations to the
RequestExtraData structure. We use this flag to determine
whether the WebFrameTestClient::willSendRequest() function needs to early return
or not. The assumption is that the logging would already
be done when the navigation was initiated for renderer initiated navigations. No
need to log when it is committed.

The other changes are to add functionality to the WebTestDelegate interface for
the runner to query this flag.

There is one more test marked under the same bug
http/tests/loading/redirect-methods.html. Does not appear to be the same issue.
Will debug and log
a new bug if required.

BUG=625765
==========

to

==========
PlzNavigate: Attempt to fix blink layout tests which fail due to duplicate
output from WebFrameClient::willSendRequest.

With PlzNavigate, the willSendRequest function is called in
RenderFrameImpl::BeginNavigation() and again during OnCommitNavigation()
when the request is loaded by blink. One option which was considered was to call
willSendRequest() in OnCommitNavigation(). However the
request may be blocked and hence that call may never happen.

To workaround this I added a flag to track renderer initiated navigations to the
RequestExtraData structure. We use this flag to determine
whether the WebFrameTestClient::willSendRequest() function needs to early return
or not. The assumption is that the logging would already
be done when the navigation was initiated for renderer initiated navigations. No
need to log when it is committed.

The other changes are to add functionality to the WebTestDelegate interface for
the runner to query this flag.

There is one more test marked under the same bug
http/tests/loading/redirect-methods.html. Does not appear to be the same issue.
Will debug and log
a new bug if required.

BUG=625765

Review-Url: https://codereview.chromium.org/2652123002
Cr-Commit-Position: refs/heads/master@{#446462}
Committed:
https://chromium.googlesource.com/chromium/src/+/fce540a08089df75d18cd4d719be...
==========

[commit-bot: I haz the power, 2017-01-26 21:53:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/fce540a08089df75d18cd4d719be...