OOPIFs give me duplicated calls to WebFrameClient::didFinishResourceLoad

[Łukasz]

Introduction

============

After making changes [1] to replicate dump_resource_load_callbacks across OOPIFs,

I've noticed I get 2 new failures in tests run in --site-per-process mode:

  http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny.html [ Failure ]

  http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html [ Failure ]

The failures were a little surprising to me - I was assuming that the behavior with

and without --site-per-process should be more or less the same (modulo which

process handles individual tasks).  This assumption turned out to be wrong

for the following Blink callbacks:

  - WebFrameClient::didFinishResourceLoad

  - WebFrameClient::willSendRequest

The callbacks above are called only once without --site-per-process, but are

called twice (once in each renderer process) with --site-per-process.  Example

(when running h/t/s/XFrameOptions/x-frame-options-parent-same-origin-deny.html

with --site-per-process) of some printf-style debugging:

  line1: [16285:16285:0415/161932:1233608978303:ERROR:web_frame_test_client.cc(546)] willSendRequest: frame=0x7eedf68c1df0; id=1; request.url=http://127.0.0.1:8000/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html; request.firstPartyForCookies=http://127.0.0.1:8000/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html

  line2: [16285:16285:0415/161932:1233609015992:ERROR:web_frame_test_client.cc(665)] didFinishResourceLoad: frame=0x7eedf68c1df0; id=1

  line3: [16285:16285:0415/161932:1233609035371:ERROR:web_frame_test_client.cc(546)] willSendRequest: frame=0x7eedf68cae00; id=2; request.url=http://localhost:8000/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi; request.firstPartyForCookies=http://127.0.0.1:8000/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html

  line4: [16297:16297:0415/161932:1233609082019:ERROR:web_frame_test_client.cc(546)] willSendRequest: frame=0x7ef86abc1f00; id=1; request.url=http://localhost:8000/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi; request.firstPartyForCookies=

  line5: [16297:16297:0415/161932:1233609087749:ERROR:web_frame_test_client.cc(665)] didFinishResourceLoad: frame=0x7ef86abc1f00; id=1

  line6: [16285:16285:0415/161932:1233609091692:ERROR:web_frame_test_client.cc(665)] didFinishResourceLoad: frame=0x7eedf68cae00; id=2

Note that above, line3 and line4 represent the same willSendRequest callback

inside 2 different renderer processes.  AFAICT, the only difference is that the

callback from line4 has an empty request.firstPartyForCookies().  The callback

from line4 will not appear without --site-per-process.

Similarily, line2 and line5 respresent the same didFinishResourceLoad callback

inside 2 different renderer processes.

Questions

=========

Q1: Is it expected that with --site-per-proces we get twice as many callbacks to

    WebFrameClient::didFinishResourceLoad and WebFrameClient::willSendRequest

    callbacks (2 callbacks for the same request)?

If the answer to the above question is "yes", then the next question is:

Q2: Is it okay to remove testRunner.dumpResourceLoadCallbacks() from

    the 2 affected layout tests (AFAICT test coverage / verification won't

    suffer) and live with the fact that some layout tests will always

    have different output with and without --site-per-process?

I think the answer is also "yes", although it makes me a bit sad...

Assumptions

===========

The questions above are asked with an implicit assumption that it is desirable

to replicate layout test flags across renderer processes.  I believe that this

is the case, but maybe this is something that should be opened for discussion.

Replication seems desirable for consistency - tests rely on replication of some

flags (i.e. ones that control whether to output a layout dump vs a pixel dump)

and therefore we introduced a mechanism to replicate flags across renderer

processes.  I think that for consistency it is desirable to extend replication

to all flags that can be controlled from layout tests javascript via testRunner

API.

Thanks,

Lukasz

[1] https://codereview.chromium.org/1895543002/

[Daniel Cheng]

On Fri, Apr 15, 2016 at 4:30 PM Łukasz <luk...@chromium.org> wrote:

Introduction

============

After making changes [1] to replicate dump_resource_load_callbacks across OOPIFs,

I've noticed I get 2 new failures in tests run in --site-per-process mode:

  http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny.html [ Failure ]

  http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html [ Failure ]

The failures were a little surprising to me - I was assuming that the behavior with

and without --site-per-process should be more or less the same (modulo which

process handles individual tasks).  This assumption turned out to be wrong

for the following Blink callbacks:

  - WebFrameClient::didFinishResourceLoad

  - WebFrameClient::willSendRequest

The callbacks above are called only once without --site-per-process, but are

called twice (once in each renderer process) with --site-per-process.

I'm guessing this is a visible artifact from the navigation transfer, but creis@ should confirm.

Daniel

[Charlie Reis]

[+clamy]

Ah yes, it looks like the transfer path is visible in the tests due to this.  The old renderer process will request the resource, then we tell the new renderer process to request the same resource.

This will go away with PlzNavigate.  Not sure if PlzNavigate is compatible with these tests yet, but you could try running it with --enable-browser-side-navigation to see if the problem goes away.  :)

In the meantime, it does sound like you can probably skip that step of the validation in these tests for the time being (though I'm not familiar with the tests or why the validation was there, so maybe check with an owner there).  Agreed that having a difference in behavior is unfortunate, but it should be resolved by PlzNavigate.

Hope that helps,

Charlie