Closed Bug 1523986 Opened 10 months ago Closed 27 days ago

Add more tests for Process-Switching POST loads

Categories

(Core :: DOM: Content Processes, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: Nika, Assigned: junior)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files, 2 obsolete files)

The logic added in bug 1467223 should be more thoroughly tested including situations such as those mentioned in bug 1522649, bug 1522640 and bug 1522641.

Assignee: nobody → valentin.gosu
Assignee: valentin.gosu → nobody
Fission Milestone: M2 → M3
Fission Milestone: M3 → M4
Blocks: 1490803
Assignee: nobody → kmadan
Type: enhancement → task

Unassigning from Kashav this requires as lot more context about different ways in which loads can occur, and how to test different layers of necko.

Nhi, could you please assign someone from Necko team on this?

Assignee: kmadan → nobody
Flags: needinfo?(nhnguyen)

(In reply to Neha Kochar [:neha] from comment #1)

Unassigning from Kashav this requires as lot more context about different ways in which loads can occur, and how to test different layers of necko.

Nhi, could you please assign someone from Necko team on this?

I can take the tests for Necko part, but it's not crystal clear here.

Nika, I have some questions for you, some of them might be out of topic:

(a) Looking at Bug 1522640, I'm wondering what non-Http(s) protocols you want to test.
Fetch doesn't allow the location header to be one url with non-Http(s) scheme.
https://fetch.spec.whatwg.org/#ref-for-concept-response-location-url%E2%91%A3

Or we want to test some error-handling?

(b)
Out of curiosity, why we care most about testing POST?
Looking at bug 1467223, it's subtle between the implementation and enabling POST.
Please skip this question if it's too tedious to answer.

(c)
Besides, I can cook tests like:
(i) a chain of redirections with different origins (origin A to origin B to origin C)
(ii) fetch API with POST, cors or no-cors. (expected in different process for a cross-origin redirect in fetch?)

What else is on top of your head?

Flags: needinfo?(nhnguyen) → needinfo?(nika)

(In reply to Junior [:junior] from comment #2)

(a) Looking at Bug 1522640, I'm wondering what non-Http(s) protocols you want to test.
Fetch doesn't allow the location header to be one url with non-Http(s) scheme.
https://fetch.spec.whatwg.org/#ref-for-concept-response-location-url%E2%91%A3

Or we want to test some error-handling?

I believe we do occasionally redirect to other types of URLs. For example, it is possible for a webextension to inject an artificial redirect IIRC, which could lead to the destination scheme being a data: URI.

Both us and chrome also definitely support redirecting from http URI responses to FTP URIs.

As an example, if you clone down WPT to use its server & run something like:

$ mkdir _nikatest
$ vi _nikatest/redirect.py
$ ./wpt serve --doc_root _nikatest

with redirect-handler.py containing:

def main(request, response):
    response.add_required_headers = False
    response.writer.write_status(302)
    # Did a google search for a ftp server and found this random public-looking one *shrug*
    response.writer.write_header("Location", "ftp://speedtest.tele2.net/")
    response.writer.end_headers()
    response.writer.write("")

Then you'll be able to see the load redirect to the given FTP server upon loading http://web-platform.test:8000/redirect-handler.py

(b)
Out of curiosity, why we care most about testing POST?
Looking at bug 1467223, it's subtle between the implementation and enabling POST.
Please skip this question if it's too tedious to answer.

The reason is just because it was the easiest way to see the situation. there's nothing specific about post other than that the request has to happen from the originating process and not the final process of the process switch due to the request POST data. It also makes it more clear that we can't make the same request twice, because POST loads don't take kindly to that.

(c)
Besides, I can cook tests like:
(i) a chain of redirections with different origins (origin A to origin B to origin C)
(ii) fetch API with POST, cors or no-cors. (expected in different process for a cross-origin redirect in fetch?)

We only expect document loads to change processes, fetch loads will not.

What else is on top of your head?

I worry about dynamically added redirects from extensions, loads of non-http URIs, serviceworker-intercepted loads with synthesized responses, cached & non-cached loads, cached & non-cached redirects, etc.

Flags: needinfo?(nika)
Flags: needinfo?(juhsu)

Thanks for those information, Nika. It helps a lot.
I have some minor questions here.

As an example, if you clone down WPT to use its server & run something like:

$ mkdir _nikatest
$ vi _nikatest/redirect.py

s/redirect/redirect-handler
:D

$ ./wpt serve --doc_root _nikatest


with `redirect-handler.py` containing:

def main(request, response):
response.add_required_headers = False
response.writer.write_status(302)
# Did a google search for a ftp server and found this random public-looking one shrug

We have good SEO :p

response.writer.write_header("Location", "ftp://speedtest.tele2.net/")
response.writer.end_headers()
response.writer.write("")

Then you'll be able to see the load redirect to the given FTP server upon loading `http://web-platform.test:8000/redirect-handler.py`

I'm surprised we allow non-Http(s) scheme in location header.
I definitely can do a test for this. And expect the test will be removed once we follow the spec :)

(b)
Out of curiosity, why we care most about testing POST?
Looking at bug 1467223, it's subtle between the implementation and enabling POST.
Please skip this question if it's too tedious to answer.

The reason is just because it was the easiest way to see the situation. there's nothing specific about post other than that the request has to happen from the originating process and not the final process of the process switch due to the request POST data. It also makes it more clear that we can't make the same request twice, because POST loads don't take kindly to that.

Got it, thanks.

(c)
Besides, I can cook tests like:
(i) a chain of redirections with different origins (origin A to origin B to origin C)
(ii) fetch API with POST, cors or no-cors. (expected in different process for a cross-origin redirect in fetch?)

We only expect document loads to change processes, fetch loads will not.

Cool, looks like we don't need this.

What else is on top of your head?

I worry about dynamically added redirects from extensions

Looks like it's covered by this

loads of non-http URIs,

Does it mean document.location = ftp://FTP_HOST_NAME?

serviceworker-intercepted loads with synthesized responses

I'm not an expert of sw. Let's see if I can make this test.
Do we change process every time the channel is intercepted?
As I know sw can use cache API, fetch API , combination of them or even pure sw generated response.

cached & non-cached loads, cached & non-cached redirects, etc.

Talking about cache and by (b), we're going to test GET here since we don't cache POST.

Flags: needinfo?(juhsu) → needinfo?(nika)
Assignee: nobody → juhsu

(In reply to Junior [:junior] from comment #4)

Then you'll be able to see the load redirect to the given FTP server upon loading http://web-platform.test:8000/redirect-handler.py

I'm surprised we allow non-Http(s) scheme in location header.
I definitely can do a test for this. And expect the test will be removed once we follow the spec :)

I should clarify that chrome also supports redirecting to FTP URIs, it acted the same way as us.

(c)
Besides, I can cook tests like:
(i) a chain of redirections with different origins (origin A to origin B to origin C)
(ii) fetch API with POST, cors or no-cors. (expected in different process for a cross-origin redirect in fetch?)

We only expect document loads to change processes, fetch loads will not.

Cool, looks like we don't need this.

What else is on top of your head?

I worry about dynamically added redirects from extensions

Looks like it's covered by this

That tests loading extension URIs, not performing synthetic redirects from the webRequest API (e.g. by using the redirectUrl property on BlockingResponse https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/BlockingResponse#Type)

IIRC you can bypass even more restrictions using that, e.g. I think you can redirect to a data or blob URI (might be wrong on that though).

loads of non-http URIs,

Does it mean document.location = ftp://FTP_HOST_NAME?

Yes, that would be an example.

serviceworker-intercepted loads with synthesized responses

I'm not an expert of sw. Let's see if I can make this test.
Do we change process every time the channel is intercepted?
As I know sw can use cache API, fetch API , combination of them or even pure sw generated response.

There's another bug around service workers. You can probably leave that test off if you don't understand how it works for now.

cached & non-cached loads, cached & non-cached redirects, etc.

Talking about cache and by (b), we're going to test GET here since we don't cache POST.

That's OK - we need both to work.

Flags: needinfo?(nika)

(a) Looking at Bug 1522640, I'm wondering what non-Http(s) protocols you want to test.
Fetch doesn't allow the location header to be one url with non-Http(s) scheme.
https://fetch.spec.whatwg.org/#ref-for-concept-response-location-url%E2%91%A3

I'm informed that the restriction only applies for subresource loading.
Sorry for the confusing.

Test file:// and data:

Nika, In donig P2, I'm confused with the goal of the test in this bug.
I figure out enabling useHttpResponseProcessSelection doesn't switch process for different http origins load.
But fission.autostart does switch.

(Not sure why file:// and http:// switch processes for useHttpResponseProcessSelection)

Could you hint me the relationship between the two prefs?
And what pref should be enabled under the tests in this bug?

Another finding is:
Under fission.autostart = true, do page load A->B->C and it comes out three processes instead of procA->procB->procA

Flags: needinfo?(nika)

(In reply to Junior [:junior] from comment #9)

Nika, In donig P2, I'm confused with the goal of the test in this bug.
I figure out enabling useHttpResponseProcessSelection doesn't switch process for different http origins load.
But fission.autostart does switch.

Yes, without fission enabled, all http:// URIs load in the same process, even with useHttpResponseProcessSelection.

(Not sure why file:// and http:// switch processes for useHttpResponseProcessSelection)

file URIs load in the file process, while http URIs load in the web process, so loading from one to the other would cause a process switch. This is the primary thing which changes when using useHttpResponseProcessSelection without fission, as the old-style process switches don't support form submission & process switches simultaneously.

Could you hint me the relationship between the two prefs?
And what pref should be enabled under the tests in this bug?

Another finding is:
Under fission.autostart = true, do page load A->B->C and it comes out three processes instead of procA->procB->procA

Is C same-origin with A? If it isn't, then that is intentional. With fission enabled, each origin gets its own process.

Flags: needinfo?(nika)
Blocks: resab
Status: NEW → ASSIGNED
Fission Milestone: M4 → ---
Priority: P3 → P2

Use COOP to force the process switch in redirect with different origins.
Also tests cache each reqeusts.

Attachment #9078950 - Attachment is obsolete: true

(In reply to :Nika Layzell (ni? for response) from comment #5)

(In reply to Junior [:junior] from comment #4)

Looks like it's covered by this

That tests loading extension URIs, not performing synthetic redirects from the webRequest API (e.g. by using the redirectUrl property on BlockingResponse https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/BlockingResponse#Type)

IIRC you can bypass even more restrictions using that, e.g. I think you can redirect to a data or blob URI (might be wrong on that though).

4 months late reply (facepalm)

 0:37.34 INFO Console message: [JavaScript Error: "Security Error: Content at moz-extension://UUID/redirect.html may not load or link to file:///PATH/toolkit/components/remotebrowserutils/tests/browser/dummy_page.html."]      

Looks like we block loading for some protocol. I'll post the patch later.

Attached file verification_comment13 (obsolete) —

data:text/html,%3Ch1%3EHello%2C World!%3C%2Fh1%3E is good
ftp://speedtest.tele2.net/ looks good (trigger FATAL ERROR: Non-local network connections are disabled and a connection attempt to speedtest.tele2.net (90.130.70.73) was made.)
blob://test is not good

Let's test with data://

Attachment #9101735 - Attachment is obsolete: true
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3674fdef6f51
P1 test process switch between protocols r=nika
https://hg.mozilla.org/integration/autoland/rev/b13610595238
P2 wpt test with multi-origins redirect and cache r=valentin,annevk
https://hg.mozilla.org/integration/autoland/rev/b0b7702d3923
P3 test process switch trigger by web extension r=valentin
https://hg.mozilla.org/integration/autoland/rev/f885e475cc2c
P4 Use extension to redirect to different protocols r=valentin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19761 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot

FYI one of the tests that was added here has non-deterministic output on failure (at least in Chromium): html/cross-origin-opener-policy/popup-redirect-cache.https.html

See https://crbug.com/1016762, https://test-results.appspot.com/data/layout_results/linux-rel/223087/webkit_layout_tests%20%28with%20patch%29/layout-test-results/results.html is an example case where the test failed twice with different outputs.

It would be great if the test could produce deterministic output on failure :)

(In reply to smcgruer from comment #22)

FYI one of the tests that was added here has non-deterministic output on failure (at least in Chromium): html/cross-origin-opener-policy/popup-redirect-cache.https.html

See https://crbug.com/1016762, https://test-results.appspot.com/data/layout_results/linux-rel/223087/webkit_layout_tests%20%28with%20patch%29/layout-test-results/results.html is an example case where the test failed twice with different outputs.

It would be great if the test could produce deterministic output on failure :)

Fair suggestion. We use uuid for convenience, which could be fixed for a gold star.
https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/testing/web-platform/tests/html/cross-origin-opener-policy/popup-redirect-cache.https.html#55-68

Could you file another bug to address this?

Depends on: 1592477

(In reply to Junior [:junior] from comment #23)

(In reply to smcgruer from comment #22)

FYI one of the tests that was added here has non-deterministic output on failure (at least in Chromium): html/cross-origin-opener-policy/popup-redirect-cache.https.html

See https://crbug.com/1016762, https://test-results.appspot.com/data/layout_results/linux-rel/223087/webkit_layout_tests%20%28with%20patch%29/layout-test-results/results.html is an example case where the test failed twice with different outputs.

It would be great if the test could produce deterministic output on failure :)

Fair suggestion. We use uuid for convenience, which could be fixed for a gold star.
https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/testing/web-platform/tests/html/cross-origin-opener-policy/popup-redirect-cache.https.html#55-68

Could you file another bug to address this?

Filed 1592477

You need to log in before you can comment on or make changes to this bug.