For HTTP/2 statusText (both Fetch and XMLHttpRequest) should always be the empty byte sequence
Categories
(Core :: DOM: Networking, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox118 | --- | fixed |
People
(Reporter: annevk, Assigned: twisniewski)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file, 1 obsolete file)
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 4•7 years ago
|
||
Updated•7 years ago
|
Updated•7 years ago
|
Reporter | ||
Comment 6•7 years ago
|
||
Comment 8•7 years ago
|
||
Reporter | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
Reporter | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Reporter | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•6 years ago
|
||
Anne, is a decision here something we should prioritize? Based on the discussion, I'm not sure if it would be worthwhile to start with just having XHRs and Fetch Responses return a blank statusText if their channel.GetProtocolVersion == "h2"?
Reporter | ||
Comment 15•6 years ago
|
||
I think that's worthwhile doing. If we can do it at a lower-level that would be better, but not making it web-exposed seems like a win as it might become some kind of interoperability issue.
I suspect HTTP/3 and whatever comes after will also not expose this, so we might want to be forward-compatible with that in the check.
Updated•6 years ago
|
Assignee | ||
Comment 16•6 years ago
|
||
@Honza, given Anne's point in comment 15, do you know which version string we'll end up returning from channel.GetProtocolVersion for http/3? "h3"? Or should I perhaps add a protocolSupportsStatusText method to channels instead for a forward-looking check?
Assignee | ||
Comment 17•6 years ago
•
|
||
Actually, maybe it would be simplest to just change the base HTTP channel's GetResponseStatusText method (which of course XHRs and Fetch both check) instead? Something like this:
NS_IMETHODIMP
HttpBaseChannel::GetResponseStatusText(nsACString& aValue) {
+ // http/2 should return an empty string
+ nsAutoCString version;
+ Unused << GetProtocolVersion(version);
+ if (version.Equals("h2")) {
+ aValue.Truncate();
+ return NS_OK;
+ }
+
if (!mResponseHead) return NS_ERROR_NOT_AVAILABLE;
mResponseHead->StatusText(aValue);
return NS_OK;
}
And also, since the web platform tests aren't ready for this yet, I'm wondering whether it would be alright to add to netwerk/test/unit/test_http2.js, or if we should have separate tests for xhr and fetch?
Comment 18•6 years ago
|
||
@baku: do you have an answer for comment #16?
Honza
Reporter | ||
Updated•6 years ago
|
Comment 19•6 years ago
|
||
Valentin, do you know who to answer to comment 16?
Comment hidden (duplicate) |
Comment 21•6 years ago
|
||
(In reply to Thomas Wisniewski [:twisniewski] from comment #16)
[...] do you know which version string we'll end up returning from channel.GetProtocolVersion for http/3? "h3"?
I suspect it's going to be "h3". Dragana can confirm.
Or should I perhaps add a protocolSupportsStatusText method to channels instead for a forward-looking check?
That would probably be a nicer solution for this.
Updated•6 years ago
|
Reporter | ||
Comment 22•3 years ago
|
||
WPT supports H2 now and I've landed the relevant tests: https://github.com/web-platform-tests/wpt/pull/12490. There's no support for H3 on WPT so no H3 tests unfortunately.
Updated•2 years ago
|
Assignee | ||
Comment 23•1 year ago
•
|
||
Revisiting this, I've written a patch which seems like it should work, but it only passes the XMLHttpRequest versions of the tests. The fetch ones are getting an empty string from the channel's GetProtocolVersion
, rather than "h2"
, and I'm not familiar enough with the code to figure out why. For now I'll just attach a patch here in case someone else has any clues.
Comment 24•1 year ago
|
||
Hi Sunil, could you take a look at comment #23 and try to help Thomas?
Thanks.
Comment 25•1 year ago
|
||
Hello Thomas,
We should be able to get the correct version if you use the HttpBaseChannel::GetProtocolVersion instead of the overriden method in HttpChannelChild.
The HttpChannelChild returns a member variable for the protocol version which gets populated only after OnStopRequest here. AFAIK, this is the reason for empty version value.
I am not sure why we need the GetProtocolVersion
override function in HttpChannelChild.
I will check this further on Monday.
I think you can use this patch for unblocking your tests.
NS_IMETHODIMP
HttpChannelChild::GetProtocolVersion(nsACString& aProtocolVersion) {
+ if (mProtocolVersion.IsEmpty()) {
+ HttpBaseChannel::GetProtocolVersion(aProtocolVersion);
+ return NS_OK;
+ }
+
aProtocolVersion = mProtocolVersion;
return NS_OK;
}
Assignee | ||
Comment 26•1 year ago
|
||
Thanks Sunil! That patch seems to have done the trick, so once we figure out the right way to fix that, we should be able to land this:
https://treeherder.mozilla.org/jobs?repo=try&revision=672b04c71c72d0775341335b552203a5e06c733a
Comment 27•1 year ago
•
|
||
Hello Thomas,
I discussed the issue with Kershaw.
We have the overridden HttpChannelChild::GetProtocolVersion because the function because the protocol version is more reliable than the one retrieved from HttpBaseChannel::GetProtocolVersion in base class. HttpChannelChild::GetProtocolVersion populates the protocol version received via IPC during the OnStopRequest. The version received via IPC is derived from the connection info object which is only available in the parent process.
The versions retrieved from connection info represents the actual protocol used and sometimes could differ from the version in the response head.
As the fetchDriver needs this info during OnStartRequest, we have decided to pass the version info via IPC during OnStartRequest instead of OnStopRequest. I will write a bug for this will try to release a patch soon (hopefully by next week).
Assignee | ||
Comment 28•1 year ago
|
||
The fix mentioned in comment 27 has now landed, and a fresh try-run for the patch (without the work-around) seems green to me: https://treeherder.mozilla.org/jobs?repo=try&revision=505e4398203da4b7aba8c7718ea94986c5188345
So let's go for it.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 29•1 year ago
|
||
Updated•1 year ago
|
Updated•1 year ago
|
Comment 30•1 year ago
|
||
Comment 31•1 year ago
|
||
Backed out for causing XHR related mochitest failures.
Assignee | ||
Comment 32•1 year ago
|
||
Oh, there's an http2 mode in mochitests now...
It looks like I'll have to annotate that test as skipped for http2 mode, and add a second version which is skipped if not http2, with the right expectations. I'll try to get that done soon.
Comment 33•1 year ago
|
||
This happened later and turned out to also be caused by this. Needinfo you again Thomas so you won't miss this when making the patch again.
failure log
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - TEST-PASS | browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html | Message should not be blank
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - decode_error.mp4: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - auto mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()() const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - Buffered messages finished
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html | Error message in allowlist: 404: Not Found - got "", expected "404: Not Found"
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - SimpleTest.is@https://mochi.test:8888/tests/SimpleTest/SimpleTest.js:507:14
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - testPromise/</video.onerror@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:20:18
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - EventHandlerNonNull*testPromise/<@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:13:3
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - testPromise@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:9:61
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - testBody@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:35:9
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - async*@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:50:9
[task 2023-07-31T23:07:32.482Z] 23:07:32 INFO - async*@https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:48:10
[task 2023-07-31T23:07:32.484Z] 23:07:32 INFO - GECKO(1594) | [Child 1783, MediaSupervisor #1] WARNING: Error constructing decoders: file /builds/worker/checkouts/gecko/dom/media/MediaFormatReader.cpp:439
[task 2023-07-31T23:07:32.485Z] 23:07:32 INFO - GECKO(1594) | [Child 1783, MediaSupervisor #1] WARNING: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - auto mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()() const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.: file /builds/worker/checkouts/gecko/dom/media/MediaFormatReader.cpp:1774
[task 2023-07-31T23:07:32.486Z] 23:07:32 INFO - GECKO(1594) | [Child 1783, MediaDecoderStateMachine #1] WARNING: Decoder=7f058252cc00 Decode error: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - auto mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()() const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.: file /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachineBase.cpp:166
[task 2023-07-31T23:07:32.489Z] 23:07:32 INFO - GECKO(1594) | JavaScript warning: https://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html, line 14: This error message will be blank when privacy.resistFingerprinting = true. If it is really necessary, please add it to the whitelist in MediaError::GetMessage: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - auto mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()() const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.
[task 2023-07-31T23:07:32.490Z] 23:07:32 INFO - TEST-PASS | browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html | Blank error message: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - auto mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()() const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.
[task 2023-07-31T23:07:32.490Z] 23:07:32 INFO - GECKO(1594) | MEMORY STAT | vsize 10778MB | residentFast 164MB | heapAllocated 17MB
[task 2023-07-31T23:07:32.493Z] 23:07:32 INFO - GECKO(1594) | [Child 1783, Main Thread] WARNING: DispatchEvent called on non-current inner window, dropping. Please check the window in the caller instead.: file /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:4195
[task 2023-07-31T23:07:32.494Z] 23:07:32 INFO - GECKO(1594) | [Child 1783, Main Thread] WARNING: DispatchEvent called on non-current inner window, dropping. Please check the window in the caller instead.: file /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:4195
[task 2023-07-31T23:07:32.497Z] 23:07:32 INFO - TEST-OK | browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html | took 2712ms
[task 2023-07-31T23:07:32.576Z] 23:07:32 INFO - TEST-START | browser/components/resistfingerprinting/test/mochitest/test_bug1382499_touch_api.html
Assignee | ||
Comment 34•1 year ago
|
||
Thanks! I'll investigate that as well.
Assignee | ||
Comment 35•1 year ago
|
||
The second error is just as esoteric; MediaErrors will now end up with empty strings for the status text, which is not normally an issue. However, resist fingerprinting mode expects the error messages to include the default text, and so I would rather add the default 404-text for HTTP2 responses and keep the code otherwise the same, instead of allow-listing two values (one with the text and one with just the code) to keep the fingerprinting protections the same.
Comment 36•1 year ago
|
||
Comment 37•1 year ago
|
||
Backed out for causing mochitest failures on test_fetch_basic_http.html.
So far, this is the only failed test.
[task 2023-08-07T04:24:32.766Z] 04:24:32 INFO - TEST-START | dom/tests/mochitest/fetch/test_fetch_basic_http.html
[task 2023-08-07T04:24:33.070Z] 04:24:33 INFO - GECKO(3759) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmptydogoaf.mozrunner/runtests_leaks_tab_pid4048.log
[task 2023-08-07T04:24:33.135Z] 04:24:33 INFO - TEST-INFO | started process screentopng
[task 2023-08-07T04:24:33.416Z] 04:24:33 INFO - TEST-INFO | screentopng: exit 0
[task 2023-08-07T04:24:33.417Z] 04:24:33 INFO - Buffered messages logged at 04:24:33
[task 2023-08-07T04:24:33.417Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Response should not be an error for file_XHR_pass1.xml
[task 2023-08-07T04:24:33.418Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Status should match expected for file_XHR_pass1.xml
[task 2023-08-07T04:24:33.418Z] 04:24:33 INFO - Buffered messages finished
[task 2023-08-07T04:24:33.420Z] 04:24:33 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Status text should match expected for file_XHR_pass1.xml - got "", expected "OK"
[task 2023-08-07T04:24:33.420Z] 04:24:33 INFO - SimpleTest.is@https://mochi.test:8888/tests/SimpleTest/SimpleTest.js:507:14
[task 2023-08-07T04:24:33.421Z] 04:24:33 INFO - testURL/</p<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:18:9
[task 2023-08-07T04:24:33.421Z] 04:24:33 INFO - promise callback*testURL/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:12:36
[task 2023-08-07T04:24:33.422Z] 04:24:33 INFO - testURL@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:11:13
[task 2023-08-07T04:24:33.422Z] 04:24:33 INFO - promise callback*runTest@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:261:6
[task 2023-08-07T04:24:33.422Z] 04:24:33 INFO - windowTest/</scriptEl.onload@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:127:9
[task 2023-08-07T04:24:33.422Z] 04:24:33 INFO - EventHandlerNonNull*windowTest/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:126:7
[task 2023-08-07T04:24:33.422Z] 04:24:33 INFO - windowTest@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:123:12
[task 2023-08-07T04:24:33.423Z] 04:24:33 INFO - testScript/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:139:14
[task 2023-08-07T04:24:33.423Z] 04:24:33 INFO - promise callback*testScript@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:138:6
[task 2023-08-07T04:24:33.423Z] 04:24:33 INFO - @https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.html:19:11
[task 2023-08-07T04:24:33.424Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Response url should match request for simple fetch for file_XHR_pass1.xml
[task 2023-08-07T04:24:33.424Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Response should have content-type for file_XHR_pass1.xml
[task 2023-08-07T04:24:33.424Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Response should not be an error for file_XHR_pass2.txt
[task 2023-08-07T04:24:33.425Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Status should match expected for file_XHR_pass2.txt
[task 2023-08-07T04:24:33.425Z] 04:24:33 INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-08-07T04:24:33.426Z] 04:24:33 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Status text should match expected for file_XHR_pass2.txt - got "", expected "OK"
[task 2023-08-07T04:24:33.426Z] 04:24:33 INFO - SimpleTest.is@https://mochi.test:8888/tests/SimpleTest/SimpleTest.js:507:14
[task 2023-08-07T04:24:33.427Z] 04:24:33 INFO - testURL/</p<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:18:9
[task 2023-08-07T04:24:33.427Z] 04:24:33 INFO - promise callback*testURL/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:12:36
[task 2023-08-07T04:24:33.427Z] 04:24:33 INFO - testURL@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:11:13
[task 2023-08-07T04:24:33.427Z] 04:24:33 INFO - promise callback*runTest@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.js:261:6
[task 2023-08-07T04:24:33.427Z] 04:24:33 INFO - windowTest/</scriptEl.onload@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:127:9
[task 2023-08-07T04:24:33.428Z] 04:24:33 INFO - EventHandlerNonNull*windowTest/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:126:7
[task 2023-08-07T04:24:33.428Z] 04:24:33 INFO - windowTest@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:123:12
[task 2023-08-07T04:24:33.428Z] 04:24:33 INFO - testScript/<@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:139:14
[task 2023-08-07T04:24:33.428Z] 04:24:33 INFO - promise callback*testScript@https://mochi.test:8888/tests/dom/tests/mochitest/fetch/fetch_test_framework.js:138:6
[task 2023-08-07T04:24:33.428Z] 04:24:33 INFO - @https://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_fetch_basic_http.html:19:11
[task 2023-08-07T04:24:33.429Z] 04:24:33 INFO - TEST-PASS | dom/tests/mochitest/fetch/test_fetch_basic_http.html | Response url should match request for simple fetch for file_XHR_pass2.txt
Assignee | ||
Comment 38•1 year ago
•
|
||
Hmm. It looks like I've missed another test or two which need to be split up so that they run differently in h2 mode (dom/tests/mochitest/fetch/test_fetch_basic_http.html and dom/tests/mochitest/fetch/test_fetch_basic_http_sw_empty_reroute.html). I'll try again when I have some more time.
Comment 39•1 year ago
|
||
Comment 40•1 year ago
|
||
bugherder |
Assignee | ||
Updated•1 year ago
|
Description
•