I've confirmed that this is a cross-origin access 1) before the https://crrev.com/c/767627 and 2) without --site-per-process.  I have also confirmed that this is a cross-origin access with(out) 1+2 and also without 3) virtual/mojo-loading.

Therefore:

- It appears that this is a problem with the test - the test is inherently broken in presence of OOPIFs and should be fixed to support OOPIFs (i.e. the test should not access cross-origin document body).

- This problem should not block landing of https://crrev.com/c/767627

falken@, could you please take ownership or triage of this OOPIF support bug in one of service-worker-related layout tests?  Can the test be refactored to avoid cross-origin document access?

Where do file:// origins even come from for this test? Are you saying the test runner is broken and incorrectly loading the test from a file:// URL rather than from http://127.0.0.1:8000 ?

Also I don't understand how the presence of OOPIF could make a difference to the test passing? Either the access to the iframe should be allowed according to the spec, or it should be denied according to the spec. But some implementation detail in chrome shouldn't make a difference, right?

RE: #c4

AFAIK layout tests run with AllowUniversalAccessFromFileURLs which relaxes some CanAccess... checks - this is why the test works without OOPIFs (but of course relaxing of the checks doesn't help if the data the test wants to look at lives in another process - this is why the test fails with OOPIFs).

RE: #c3

It seems that the test is indeed run from file:// origin - an ad-hoc debugging output below:

    testRunner.logToStderr("fetch-request-body-file.html:138: window.origin = " + window.origin);
    async_test(function(t) {

prints:

    fetch-request-body-file.html:138: window.origin = file:/


I wonder if the directory of the test has any special meaning: http/tests/***local***/serviceworker/...  OTOH, so far I haven't been able to find code that special-cases "local" subdirectory here.

Ah, good point. 10 years ago when that directory was created with the following commit message: "Added a new category of http tests - "local" where the test is run as a local file but the test involves remote resources from the httpd.", so seems like loading from file:// is indeed expected. Never mind me then.

(and the CL that originally added this test claims that it has to be in that directory to be able to use eventSender.beginDragWithFiles(), which seems to be because of https://bugs.webkit.org/show_bug.cgi?id=25909)

This sounds related to issue 616626 and today's  issue 786539 .  It sounds like they're all due to a question of whether AllowUniversalAccessFromFileURLs can be supported in --site-per-process?

Hm as per the comments here this looks like a historical limitation of http tests, that they need to run in http/local in order to use beginDragWithFiles(), and http/local does weird things with origins.

Looks like an interesting bug.

This test is important because it looks like our only test that actually uploads a file to a service worker, which is slightly broken  in ServiceWorkerServicification at  issue 916070 .

After trying to write a test for that bug, I indeed found beginDragWithFiles() doesn't work well in the http directory (needed for service worker) but this "local" test somehow works.

beginDragWithFiles() seems like the only way to make an automated test that uploads a file from a form.

The WebKit bug mentions relative path as a problem but it doesn't seem like the full story because even hardcoding an aboslute path on my system didn't result in a file element added to the request body.

Needs more investigation.

RE: beginDragWithFiles() seems like the only way to make an automated test that uploads a file from a form.

Is it an option to author a browser test instead?  (e.g. search for FileChooserDelegater in browser tests under //content).

If we want to keep it as a layout test, then maybe we can add some special hooks into testRunner (maybe something like testRunner.fillInputFieldWithPath(inputFileElementId, relativeTestPath))?

BTW: Do WPT offer a way to upload a test file?


PS. Note that issue 615575 says "it is known that drag-n-drop does not work with OOPIF [for selecting files via form input;  it should work in other scenarios]".

Thanks for the pointer to FileChooserDelegate. That may be the best way to do this.

WPT has a kind of a hack where it uses fetch() instead of submitting the form, I'm not sure it will give adequate test coverage:
https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/infrastructure/testdriver/file_upload.sub.html?rcl=491e2671e35208ba3654d4211c775708df5c52f2&l=17

The test is also marked [ Skip ] currently and is failing locally.

Wpt supposedly does have infra for doing form file uploads. I haven't had time yet to experiment with it and figure out how well it works, but apparently there are some tests that use it already.

There is https://github.com/web-platform-tests/wpt/issues/8114 and on #testing I was directed to file_upload.sub.html. That seems like the state of the art now.

Re comment 11: It's possible my attempt to use beginDragWithFiles() outside http/local with an absolute path failed because I didn't add a "name" attribute to the <input type="file"> which apparently means it gets skipped in form submission. oops.

In  issue 916070  I made a file upload browser_test in service_worker_file_upload_browsertest.cc. I plan to move this layout test also to that browser tests so we still have test coverage with Site Isolation on.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b47bd4c46c10d9f90ed451bc693e88e4681837ee

commit b47bd4c46c10d9f90ed451bc693e88e4681837ee
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Jan 11 05:14:30 2019

service worker: Migrate file upload test from web_test to browser_test.

fetch-request-body-file.html (added in [1]) is our main test for
uploading a file via <form> and having a service worker intercept the
request. http web_tests that for uploading files need to be in
http/tests/local which runs the test from file: URLs while running the
http server for http URLs[2]. This was causing cross-origin accesses
which is normally okay because file: is whitelisted in web_tests, but
with Site Isolation on the content ends up in different processes and
the test breaks. Therefore the test has been disabled by default and
only running in the virtual suite with Site Isolation enabled.

This CL starts moving the test to a browser_test so it can be enabled by
default. There is remaining work to add the Network Fallback test case.

[1] https://codereview.chromium.org/491203002
[2] https://bugs.webkit.org/show_bug.cgi?id=25909

Change-Id: I6e0349bce4df903f105bafb50b8ae81fcbe0ed4e
Bug:  786510 
Reviewed-on: https://chromium-review.googlesource.com/c/1405110
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621914}
[modify] https://crrev.com/b47bd4c46c10d9f90ed451bc693e88e4681837ee/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/b47bd4c46c10d9f90ed451bc693e88e4681837ee/content/test/data/service_worker/file_upload_worker.js
[modify] https://crrev.com/b47bd4c46c10d9f90ed451bc693e88e4681837ee/content/test/data/service_worker/form.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c956f49493e27a5f22167d729a4c57ecd740074e

commit c956f49493e27a5f22167d729a4c57ecd740074e
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jan 16 01:53:18 2019

service worker: Migrate file upload test from web_test to browser_test (pt 2).

This finishes the work of r621914
<https://chromium-review.googlesource.com/c/1405110>.

The web_test is removed.

Bug:  786510 
Change-Id: I53ac9f2af3e1eb7bfc1ae98ee6e863915d1efe0d
Reviewed-on: https://chromium-review.googlesource.com/c/1410794
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623008}
[modify] https://crrev.com/c956f49493e27a5f22167d729a4c57ecd740074e/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/c956f49493e27a5f22167d729a4c57ecd740074e/content/test/data/service_worker/file_upload_worker.js
[add] https://crrev.com/c956f49493e27a5f22167d729a4c57ecd740074e/content/test/data/service_worker/file_upload_worker.js.mock-http-headers
[modify] https://crrev.com/c956f49493e27a5f22167d729a4c57ecd740074e/third_party/blink/web_tests/TestExpectations
[modify] https://crrev.com/c956f49493e27a5f22167d729a4c57ecd740074e/third_party/blink/web_tests/VirtualTestSuites
[delete] https://crrev.com/bfb4db2020de72fcfe32e5842cedabc9c7eb8613/third_party/blink/web_tests/http/tests/local/serviceworker/OWNERS
[delete] https://crrev.com/bfb4db2020de72fcfe32e5842cedabc9c7eb8613/third_party/blink/web_tests/http/tests/local/serviceworker/fetch-request-body-file.html
[delete] https://crrev.com/bfb4db2020de72fcfe32e5842cedabc9c7eb8613/third_party/blink/web_tests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html
[delete] https://crrev.com/bfb4db2020de72fcfe32e5842cedabc9c7eb8613/third_party/blink/web_tests/http/tests/local/serviceworker/resources/fetch-request-body-file-test.php
[delete] https://crrev.com/bfb4db2020de72fcfe32e5842cedabc9c7eb8613/third_party/blink/web_tests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js

I "solved" the issue by moving the web_test to a browser_test.

Alternatives considered included changing the test so that the file: page and http: page communicate by postMessage, but it would be a bit clunky to inject that logic into the response for the form submission which is currently JSON echoing/describing the request. Another possibility was to fix https://bugs.webkit.org/show_bug.cgi?id=25909 so that beginDragWithFiles would work outside http/tests/local, which in the end may have been less work.

I guess we can revisit if proper file uploading test support is added to WPT, then make this a WPT test.

Upgrading this to P2 as we were losing test coverage since the test was disabled for Site Isolation, and it turns out testing file uploads under Site Isolation is important, see  bug 916070 .