In PHP, the Last-Modified header will send with any format of date, even if the date-string is blank it will send a blank Last-Modified header. When Python encounters a non-standard/blank date-string, it will not send the Last-Modified header. This test tests expectations with standard and non-standard dates. We need to change the test to match the expectations from Python with how it handles non-standard dates in the header.
<rdar://problem/77020039>
Created attachment 426837 [details] Patch
Comment on attachment 426837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426837&action=review > LayoutTests/http/tests/misc/last-modified-parsing-expected.txt:11 > Fri, 21 Nov 2008 01:03:33 GMT Removed untested dates > LayoutTests/http/tests/misc/last-modified-parsing.html:12 > test('Tuesday, 21 Nov 2008 01:03:33 GMT'); Removed non-standard dates
We want to test how WebKit software (the whole stack, including CFNetwork) handles these date formats. I understand that the Python library we are using makes it difficult to test this, but that doesn’t remove the need to do the testing. How will we do this testing, now?
(In reply to Darin Adler from comment #4) > We want to test how WebKit software (the whole stack, including CFNetwork) > handles these date formats. I understand that the Python library we are > using makes it difficult to test this, but that doesn’t remove the need to > do the testing. How will we do this testing, now? This actually isn't a limitation of Python. It's a limitation of Apache, which refuses to serve what it considers invalid date formats coming from CGI scripts. Short of building our own webserver, I'm not sure what can be done. Apache could end up taking away this ability from PHP in a future update as well. As reflected in Chris's EWS run, Apache on Windows looks like it has a more strict set of rules than on other platforms.
(In reply to Jonathan Bedard from comment #5) > (In reply to Darin Adler from comment #4) > > We want to test how WebKit software (the whole stack, including CFNetwork) > > handles these date formats. I understand that the Python library we are > > using makes it difficult to test this, but that doesn’t remove the need to > > do the testing. How will we do this testing, now? > > This actually isn't a limitation of Python. It's a limitation of Apache, > which refuses to serve what it considers invalid date formats coming from > CGI scripts. Short of building our own webserver, I'm not sure what can be > done. Apache could end up taking away this ability from PHP in a future > update as well. As reflected in Chris's EWS run, Apache on Windows looks > like it has a more strict set of rules than on other platforms. OK. So we can’t use Apache for this. How can we test this aspect of WebKit networking behavior?
(In reply to Darin Adler from comment #6) > (In reply to Jonathan Bedard from comment #5) > > (In reply to Darin Adler from comment #4) > > > We want to test how WebKit software (the whole stack, including CFNetwork) > > > handles these date formats. I understand that the Python library we are > > > using makes it difficult to test this, but that doesn’t remove the need to > > > do the testing. How will we do this testing, now? > > > > This actually isn't a limitation of Python. It's a limitation of Apache, > > which refuses to serve what it considers invalid date formats coming from > > CGI scripts. Short of building our own webserver, I'm not sure what can be > > done. Apache could end up taking away this ability from PHP in a future > > update as well. As reflected in Chris's EWS run, Apache on Windows looks > > like it has a more strict set of rules than on other platforms. > > OK. So we can’t use Apache for this. > > How can we test this aspect of WebKit networking behavior? Alexey pointed out there is an Apache setting, `send-as-is`, which may let us work-around this. If that fails, the next step would be looking at the wpt webserver and see if that lets us test what we're looking to.
https://httpd.apache.org/docs/current/mod/mod_asis.html This cannot perform any processing of course, it's just a fixed response.
AsIs still changes these dates.
(In reply to Jonathan Bedard from comment #7) > If that fails, the next step would be looking at the wpt webserver and see > if that lets us test what we're looking to. This definitely works with wptserve. Note that the server has a bunch of built-in support for things like varying headers: https://web-platform-tests.org/writing-tests/server-pipes.html#headers, so we can just replace last-modified.php with an HTML file. (Ideally we'd rewrite this to use testharness.js with moving it to http/wpt, but that's a larger change!)
Created attachment 428297 [details] Patch
Comment on attachment 428297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428297&action=review > LayoutTests/http/wpt/misc/last-modified-parsing.js:6 > +} Any reason we didn't put this inside http/wpt/misc/no-last-modified.html? > LayoutTests/http/wpt/misc/no-last-modified.js:4 > +}, "Last-Modified sent with no date"); Any reason we didn't put this inside http/wpt/misc/no-last-modified.html? > LayoutTests/platform/win/TestExpectations:-4410 > -webkit.org/b/209455 http/tests/misc/last-modified-parsing.html [ Failure ] Pretty sure Windows will still struggle with this, although I suppose it's possible that is was actually Apache that was the problem, and not the engine, I guess EWS will let us know
Comment on attachment 428297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428297&action=review > LayoutTests/http/wpt/misc/last-modified-parsing.js:1 > +function test(date) { Let's not call this function test, as that redefines the test function defined in testharness.js; maybe test_last_modified or something? > LayoutTests/http/wpt/misc/no-last-modified.js:3 > + assert_greater_than((new Date(Date.parse(actual))).getTime(), (new Date()).getTime() - (24+1)*60*60*1000); We probably want `assert_approx_equals` with an epsilon of maybe 1 minute? >> LayoutTests/http/wpt/misc/no-last-modified.js:4 >> +}, "Last-Modified sent with no date"); > > Any reason we didn't put this inside http/wpt/misc/no-last-modified.html? Additionally, is there any reason why we don't just put this into last-modified-parsing.html? It leads to us having last-modified-utilities.sub.js as a separate file, and it's not clear what the benefit here is? It's still about parsing a header value, just an empty one. > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:1 > +function observeResources(n) { This function is dead code. > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:24 > + iframe.addEventListener('error', reject); We don't want the error event here (there's no error event ever fired on an iframe, far as I can tell) > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:26 > + iframe.src = "/common/blank.html?pipe=header(Last-Modified," + header_value.replace(' ', '%20').replace(',', '\\,') + ")"; What's the reason for the header_value.replace(' ', '%20') (which the browser's URL parsing will do anyway), versus something broader like encodeURI/encodeURIComponent? > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:32 > +} This file doesn't need .sub. in its name. (See also above about merging no-last-modified and last-modified-parsing, which would mean this file could then be merged into last-modified-parsing.)
Created attachment 428413 [details] Patch
(In reply to Sam Sneddon [:gsnedders] from comment #13) > Comment on attachment 428297 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428297&action=review > > > LayoutTests/http/wpt/misc/last-modified-parsing.js:1 > > +function test(date) { > > Let's not call this function test, as that redefines the test function > defined in testharness.js; maybe test_last_modified or something? > > > LayoutTests/http/wpt/misc/no-last-modified.js:3 > > + assert_greater_than((new Date(Date.parse(actual))).getTime(), (new Date()).getTime() - (24+1)*60*60*1000); > > We probably want `assert_approx_equals` with an epsilon of maybe 1 minute? > > >> LayoutTests/http/wpt/misc/no-last-modified.js:4 > >> +}, "Last-Modified sent with no date"); > > > > Any reason we didn't put this inside http/wpt/misc/no-last-modified.html? > > Additionally, is there any reason why we don't just put this into > last-modified-parsing.html? It leads to us having > last-modified-utilities.sub.js as a separate file, and it's not clear what > the benefit here is? It's still about parsing a header value, just an empty > one. > > > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:1 > > +function observeResources(n) { > > This function is dead code. > > > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:24 > > + iframe.addEventListener('error', reject); > > We don't want the error event here (there's no error event ever fired on an > iframe, far as I can tell) > > > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:26 > > + iframe.src = "/common/blank.html?pipe=header(Last-Modified," + header_value.replace(' ', '%20').replace(',', '\\,') + ")"; > > What's the reason for the header_value.replace(' ', '%20') (which the > browser's URL parsing will do anyway), versus something broader like > encodeURI/encodeURIComponent? > > > LayoutTests/http/wpt/misc/resources/last-modified-utilities.sub.js:32 > > +} > > This file doesn't need .sub. in its name. (See also above about merging > no-last-modified and last-modified-parsing, which would mean this file could > then be merged into last-modified-parsing.) These as well as comment 12's comments addressed and fixed in comment 14
Comment on attachment 428413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428413&action=review > LayoutTests/platform/win/TestExpectations:4410 > +webkit.org/b/209455 http/wpt/misc/last-modified-parsing.html [ Failure ] Looks like the test failure on Windows EWS sees is this same bug
Created attachment 428424 [details] Patch for landing
Committed r277408 (237658@main): <https://commits.webkit.org/237658@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428424 [details].