Send strong ETags for partial content

Please test...

I'll look at this for peer review tomorrow. Had a quick glance now and everything looks good.

Oh, one comment. Is it worth making the $etag parameter optional and defaulting to null or ''? This will break compatibility for any plugins out there serving files themselves for some reason.

Hi, yes, I was considering that too, but because we can not print any debug messages and the error output must be already disabled in these scripts via "define('NO_DEBUG_DISPLAY', true);" this should not create any problems, it is just a notice that will go to error_log, right?

I agree with you Re the default parameter value - that makes some sense, though because we do set NO_DEBUG_DISPLAY, it may mean that plugin developer may never discover the bug :\

[Y] Syntax
[Y] Output
[Y] Whitespace
[-] Language
[-] Databases
[N] Testing
[Y] Security
[-] Documentation
[Y] Git
[N] Sanity check

Testing

Needs some testing instructions

Sanity check

General:
Just wondering why you're dropping both ETag and Etag? According to RFC 2616 Section 4.2, "Field names are case-insensitive.". The documentation on header_remove also states that the parameter name is case insensitive.

lib/xsendfile.php:
Also I think it would make more sense to move your new header_etag function out of xsendfilelub.php and into somewhere more appropriate - perhaps lib/weblib.php. Sending etags is not related to XSendFile really.

I guess it makes sense to use header rather than header_etag() when you know that there won't be any byte serving in use.

Other than the points above, this looks good and a much welcomed addition

Or was that a request for me to test and write instructions?

Grrr - I read header_remove() was case-sensitive, fixing...

header_etag() can not be in core libs because it is in pages that do not have any core libs available, all they have is CFG from config.php

I think about the hader_etag() once more, but so far I did not find better place for it.

Thanks!

Repo updated, I have used the name "_header_etag" to mark it private and I have also added some more inline docs. The tag removing is now fixed too. Thanks a lot.

Hi All,
I'm keep on reading RFCs and some code: my last comment has been written in https://tracker.moodle.org/browse/MDL-39688?focusedCommentId=224831&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-224831.

I must admit that I'm somewhat confused, sad for my ignorance: right now I'm thinking the opposite compared to my own comment linked above that is to say I'm thinking the same way as Andrew does when he quotes http://tools.ietf.org/html/rfc2616#section-13.5.4: ETag must be always related to the full content representing the URL being requested, served partially or not.

To enforce Andrew's point http://tools.ietf.org/html/rfc2616#section-14.27 is more explicit about the ETag to be related with the entire content and not to the partial response, indeed If-Range should do the trick for an outdated partial response.
But... we've evidence that ETag for partial content breaks things as per Andrew's use case: I'm totally lost... unless there is an issue in the way that MP4 client is able to cache partial responses.

ETag in Moodle will always be strong being equal to the content hash and when the content change ETag will change too. Should Moodle implement the support for $_SERVER['HTTP_IF_RANGE'] too (limited or not to X-Sendfile), re-sending the whole file in case of mismatching?
Maybe sniffing the traffic to reconstruct HTTP Requests and HTTP Responses between the two parts while generating the issue, as described in MDL-39688, will help.

My apologise for the noise I've created 'till now.

Truly,
Matteo

I am also confused a bit after reading the spec, the reality in proxies and client might be different unfortunately.

I am not sure my proposed patch is the correct solution, this was the main reason why I created this issue, I do not think we should rush this through integration now.

Thanks everybody!

I'm glad that I'm not the only one confused then Matteo Scaramuccia!

I've just tried your patch Petr, and it still doesn't work as expected - MP4 only plays within Chrome when we don't return any ETag headers with the byte-served content it seems.
Still investigating.

This only seems to affect when using X-Accel-Redirect (and possibly other X-Sendfile implementations). It does not seem to affect the stock Moodle byte-serving.

In fact, it's fine with nginx proxying to an apache server right up until you enable the X-Sendfile setting.

Just to confirm, this is also broken with mod_xsendfile on Apache with ETags present both with, and without the range. That's just straight Apache2 -> mod_php5 so no proxying going on.

The only browser I've been able to test in so far is Chrome. It seems that neither Firefox, nor Opera seem to request partial content for mp4 content (grr) so they don't seem to exhibit this bug (yay). They may of course request partial content for other content types but I've not tried that.

Matteo Scaramuccia and Petr Škoda, what browsers + content are you testing this with?

Hi Andrew,
'till now I've just read specs and code, nothing more: I was waiting for any feedback about what would be the client showing that strange and blocking issue.
I'll play with CR and MP4 plus I'd create some tests using curl to verify Moodle supporting byte serving - which is by far working "on average" - even with If-Range - currently missing, I'm thinking to propose its support for 2.6 - and I'll look at the HTTP Headers exchanged using CR, to identify what could be the issue in terms of HTTP specs compliance or CR specific issue. Unfortunately this work will be done on spare time, typically evening.

EDIT @20130528T0925+02:00: have you already checked that the byte range provided by CR is a real partial response request i.e. not what described here, http://stackoverflow.com/questions/12801192/client-closes-connection-when-streaming-m4v-from-apache-to-chrome-with-jplayer#answer-12979414? It could be where Moodle byte serving could be improved in replying to a "fake Partial Response" request.

Matteo: Do you want this to be assigned to you? I am not sure I would find enough time to create a solution based on reverse engineering of proxies and clients.

Or Andrew, do you want this?

Hi Petr,
my time is unfortunately limited, maybe Andrew's time for this issue could be more than mine: I'll be happy to help in any case as per my capabilities and available time.

I can try and look into this further in about two weeks time.

Here is my patch, anybody is free to work on this issue: https://github.com/skodak/moodle/compare/wip_MDL-39832_m26_etagranges

Thanks everybody!

Hi Andrew,
this weekend I'll do some investigations&coding, could you provide me a link to an MP4 sample file "big" enough to replicate the issue using Chrome on master running under Apache 2 with mod_xsendfile?
One other potential issue is that ETag must be double quoted, http://tools.ietf.org/html/rfc2616#section-3.11:

      entity-tag = [ weak ] opaque-tag
      weak       = "W/"
      opaque-tag = quoted-string

Even if I'll be unsuccessful I'll provide a WIP branch to show how the things should be IMHO changed according with RFC2616.

EDIT: in the mean time I've found some MP4 files here, http://www.thriveforums.org/forum/toshiba-thrive-video/3418-sample-mp4-videos-thrive.html, converted from MOV files.

TIA,
Matteo

Hi Matteo,

I'm not using anything special - I'm just using a free sample MP4 I found somewhere. It doesn't have to be anything long. I uploaded it to a section description, then viewed it in Chrome using the link it displays as to entirely rule out an issue with the Media plugin. I.e. I'm opening pluginfile.php directly. Make sure you've not disabled the browser cache (in Chrome Dev tools).

Hi All,
I've done several experiments using Apache/2.2.3, mod_xsendfile/0.12 and Chrome with the provided MP4 files (http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 included) changing things as per my previous comments ending with adding double quotes: this last change did the trick.

@Andrew Nicols: could you test it?
My suggestion is to apply this change in all the branches where MDL-39688 has been applied: I'll be happy to provide them including test instructions.

Wow, is it really only the missing double quotes? If yes should we fix the rest of etags? Are there any other headers that require double quotes?

Hi Petr,
it looks like that: would you like to test it too, reverting first MDL-36888 to see that Chrome will not be happy and then applying my proposal? In my environment works as described, but you know, I could be wrong
I'll double check RFC2616, while waiting for comments from Andrew, for other potential syntax issues.

Quick searched for ((^|\s)header([^LH]+); - No redirect (Location) and HTTP/1.0 statuses -, here the comments on the matches:

• Accept-Ranges: no quotes
• Cache-Control: no quotes, just for extensions. Not applicable in Moodle
• Content-Disposition, filename must be quoted: not always in Moodle e.g. grade/edit/outcome/export.php
• Content-Encoding: no quotes
• Content-Length: no quotes
• Content-Type: no quotes. Almost OK except for:
  • LF in lib/tablelib.php. Same in mod/survey/download.php
  • charset should not be quoted e.g. not Content-Type: text/xml; charset="utf-8"': few files affected
• Etag: quotes required. Missing quotes in: lib/csslib.php, theme/image.php, theme/yui_image.php, ...
• Expires: no quotes
• Pragma: no quotes except for extensions. Not applicable in Moodle
• Status: no quotes
• Last-Modified: no quotes (HTTP-date)
• WWW-Authenticate: quotes for Realm (RFC2617)

If Andrew and you will confirm my current proposal to work as expected in your environments, I'd suggest to fix all the other ETag headers, e.g. in lib/csslib.php, as part of a separate improvement for 2.6, not sure for the previous versions.
Note: FirePHP::setHeader doesn't apply quotes for headers BUT it is not used to set ETag. Similar comment for XHProf wrapper.

Thanks Matteo - I can't believe it was this simple. I've spent a couple of hours trying to track it down and re-read the RFC numerous times. Kudos

I've just tried wrapping in quotes and it does indeed work as expected. Your patch looks good to me.

I think that we should correct the quotes in the other ETag headers you've identified as part of this patch. The Byte Range issues are just a symptom of the real issue. This should also be backported to all stable branches IMO.

I think we should file a new issue for the other header quoting issues you've identified rather than as part of this one.

Andrew

+1 for the backport

OK Andrew&Petr, will do one of my evening (CEST).

Hi All,
here are the backports of the current proposal.

Do you agree that fixing the other missing quotes could be done in a separate issue? For clarity sake, this issue should IMHO cover just what discussed and done in MDL-39688 but I'll wait for your thoughts.

submitting, +1 for separate issue for the remaining quotes, thanks!!

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Rebased.

Great detective work Matteo!

I've integrated this to master, 25, 24 and 23 - thanks!

The file doesn't play any more on chrome. The content of file is empty always. This works perfectly on stable.
Let me know if I can provide more info on anything.
Dan has confirmed the issue.
Failing.

To try and add some detail, there are definite scenarios where chrome seems to not load the file when requested directly, but I HAVE seen it load when requested by quicktime. Either way, I have to say it does look to be something fishy with the etag

Hi Ankit&Dan,
could you give me some details about your tests to let me replicate it in my environment? Here are the questions about what I'm supposing to be required to identify the scope of your tests:

1. Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions
2. Could you share the movie used for your tests?
3. Could you confirm that tests has been performed using the pluginfile.php URL?
4. Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between?
5. Did you use X-Sendfile configuration or just "plain" PHP module connection?
6. Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it )

> 1. Before starting with tests, did you cleanup Chrome cache? It's unfortunately a missing point in the testing instructions

Yes - well I tried in incognito window

> Could you share the movie used for your tests?

We used the mp4 in the testing instructions, and I also used http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4

> Could you confirm that tests has been performed using the pluginfile.php URL?

Yep

> Could you confirm that the connection between your browser and the Moodle instance is direct e.g. a local installation i.e. no proxy in between?

Yep

> Did you use X-Sendfile configuration or just "plain" PHP module connection?

No X-Sendfile

> Did you check what both Fiddler and Chrome Dev Tools show during your tests to identify any eventual other reason? (I know, this is the part I must work on... when I'll replicate it )

No fiddler here as I don't use windows.

(i'm happy to sniff the traffic to work out whats going on, but I am not I can)

Hi Matteo,
Not sure am being of much help here, but anyway:-

1. No, I didn't . But I just retried with clean cache, same issue.
2. I used the one you mentioned http://samples.mplayerhq.hu/MPEG-4/vorbis-in-mp4/mi2_vorbis51.mp4 and the one here http://www.quirksmode.org/html5/tests/video.html
3. Yes
4. Yes
5. Yes (I did see x send file tag in header)
6. Am on linux, so used only the dev tools. I couldn't think of any other reason, why the contents will be empty. Anyways here are the headers, see if it helps

   GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1
   Host: ankit.moodle.local
   Connection: keep-alive
   Cache-Control: max-age=0
   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
   User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
   Referer: http://ankit.moodle.local/int/master/moodle/login/index.php
   Accept-Encoding: gzip,deflate,sdch
   Accept-Language: en-GB,en-US;q=0.8,en;q=0.6
   Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0
   HTTP/1.1 200 OK
   Date: Thu, 13 Jun 2013 02:17:29 GMT
   Server: Apache/2.2.22 (Ubuntu)
   X-Powered-By: PHP/5.3.10-1ubuntu3.6
   Expires: Fri, 14 Jun 2013 02:17:29 GMT
   Cache-Control: max-age=86400
   Pragma: 
   Content-Disposition: inline; filename="big_buck_bunny.mp4"
   Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT
   Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e"
   Accept-Ranges: bytes
   X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e
   Content-Length: 0
   Keep-Alive: timeout=5, max=98
   Connection: Keep-Alive
   Content-Type: video/mp4
   
   

   GET /int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4 HTTP/1.1
   Host: ankit.moodle.local
   Connection: keep-alive
   Accept-Encoding: identity;q=1, *;q=0
   User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
   Accept: */*
   Referer: http://ankit.moodle.local/int/master/moodle/pluginfile.php/418/mod_resource/content/1/big_buck_bunny.mp4
   Accept-Language: en-GB,en-US;q=0.8,en;q=0.6
   Cookie: MoodleSession=nk41ksu6f8qrggotnbnnbu3pd4; MOODLEID1_=%25DB%25CF%2592%25FBU%25F3%258C; MOODLEID1_=%2598%2529%25B2%25B0%25ED2%252B0
   Range: bytes=0-
   
   HTTP/1.1 200 OK
   Date: Thu, 13 Jun 2013 02:17:29 GMT
   Server: Apache/2.2.22 (Ubuntu)
   X-Powered-By: PHP/5.3.10-1ubuntu3.6
   Expires: Fri, 14 Jun 2013 02:17:30 GMT
   Cache-Control: max-age=86400
   Pragma: 
   Content-Disposition: inline; filename="big_buck_bunny.mp4"
   Last-Modified: Wed, 12 Jun 2013 07:33:28 GMT
   Etag: "0181d7880acf0c443a57cd2510a5b81447fa577e"
   Accept-Ranges: bytes
   X-Sendfile: /var/moodledata/int/master/filedir/01/81/0181d7880acf0c443a57cd2510a5b81447fa577e
   Content-Length: 0
   Keep-Alive: timeout=5, max=97
   Connection: Keep-Alive
   Content-Type: video/mp4
   
   

   Thanks

TNX for your replies!
About sniffing and options out of a Windows env: WireShark is The Network Sniffer, regardless the OS. You can inspect everything, please filter the traffic limiting it at least to HTTP otherwise you'll be lost in a sea of packets . You'll find several hints in the network e.g. http://www.eriky.com/2009/01/sniffing-http-headers-with-wireshark.
In *nix systems, especially when troubleshooting on the server, tcpdump is a great (and powerful as well) CLI.
Fiddler is a really nice and powerful tool for Windows, focused on HTTP requests, easy to use almost for any user: that's the reason why I suggested it, my fault not to consider other options.

I'll perform tests&investigations against your video one of the next evening, unfortunately out of this integration week.

@Ankit Agarwal: when you configure X-Sendfile in a Moodle deployment you need first to install its support in your web deploy: here, you need to install mod_xsendfile since you're running Moodle under Apache. The goal of that extension is, shortly, to:

• look for the X-Sendfile header
• pickup the path to your local system
• REMOVE it from your HTTP Headers otherwise, big disclosure here!
• serve your file directly, offloading the PHP module i.e. Moodle will not send anything when running under that context, expecting&trusting that the Web Server will do it as per X-Sendfile specs.

That's why you receive nothing: please, install the module in your Ubuntu e.g. http://blog.brightbox.co.uk/posts/apache-x-sendfile-module-for-ubuntu-hardy

Hi Matteo,

Yep I know how to sniff the traffic, but I don't know what you want us to look for

Anyway, i'm going to need to revert this, because from what I can see from our testing, it is continuing to cause a problem

I've reverted this change, sorry we have had to do that.

Hi Dan,

1. Sniffing: I apologize for my misunderstanding your initial point !
   BTW, what should be looked for is the correct match between Request type and given Response. If the Request contains an Accept header, the Response should be exactly related (e.g. right Content-Length, before looking at the body itself) and you need to compare the Chrome Dev Tools network information with the ones sniffed to be sure that, at the end, a Request shown in Chrome Dev Tools as served by the internal cache means no hit to the server unless the previous (partial) requests didn't cover the all content length and Chrome is correctly required to ask the server for the missing parts.
   Have you already try to cleanup your Chrome cache, perform the same test without the PR to see if your behavior is due to the PR i.e. to the usage of ETag?
2. Integration: no problem, that's the process. I hope to hear news from Ankit tests setup, too.

Hi Matteo,
I already had x-send file installed and enabled, but am not sure if it is working correctly, as I just tested without the x-sendfile config and the vid was working fine. Perhaps some configuration setup is missing, I will investigate a little more and get back to you.
Cheers

Hi Ankit,

1. Setup: please check the Apache error log and the way the module has been packaged/configured for Ubuntu i.e. you should find at least these parameters in your configuration: XSendFile on and XSendFilePath /var/moodledata/int/master, the second being important because it gives the request acceptance scope. Please note that the scope should be built using multiple XSendFilePath lines, at least two in order to map both your WWWROOT and DATAROOT.
2. Tests: could you talk offline with Dan to search for differences between both setups (when using Chrome with no X-Sendfile in Moodle) to eventually return me any feedback for my investigations?

TIA

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Hi Dan,
just finished a first test run using all the permutations of the test configurations under Chrome/27.0.1453.110 Windows/Vista SP2 i.e. including incognito windows and running the PR with and without $CFG->xsendfile = 'X-Sendfile'; against the big movie I proposed and your video: all of them successfully.
I'll try to use Chrome on a VM with Windows XP SP3, to start excluding issues with my dev/test environment.

Same successful result under Chrome/27.0.1453.110 Windows/XP SP3 using all the permutations of the test fixtures combinations above.

Stopping here waiting for some feedback from Ankit.

Hi Andrew,
I guess that the next step should be asking for a new peer review to step into another test session since everything is working on my side even under the worst - as per MDL-39688 - case, Chrome + X-Sendfile.

Wondering if you'd have the time to repeat your tests too. Since MDL-40002 has been integrated, IMHO we should at least add quotes to the current ETag related code, if we are not confident that this patch does the job in helping bandwidth saving in case of Partial Responses (HTTP 206).

Test summary:

Tester	w/o X-Sendfile	w/ X-Sendfile
Andrew/1° run
Andrew/2° run
Ankit/1° run	-
Dan/1° run		-
Matteo/1° Run
Matteo/2° Run

I've been trying to get around to testing this myself too for the past few days but haven't had a chance. The last time that I checked, it was all working correctly both with, and without X-SendFile. I also tried with nginx and X-Accel-Redirect just for completeness.

I'll have another go testing shortly.

Yup - you can add a double-tick for me too. All works as expected wit Chrome on Mac OS. Tested with one commit reverted just to prove the issue too.

TNX Andrew!
It seems we need to wait for replies from Dan/Ankit to identify the next step.

Hi Matteo,

I'm not going to have time to test this with x-sendfile in the near future (i'm running stock version of mac apache so need to compile it myself, and a bit pressed for time at the moment), but retesting without it and it seems it must've been cached or something and I am not seeing the problem when I tested. Originally I had not seen that Ankit was testing this with x-sendfile, which has lead to quite some confusion on this issue! Sorry about that.

So, I think this is good to go and was ok in the first place. I can confirm that Ankit didn't have x-sendfile properly configured (just had the module enabled), so that is likely the cause of ankits problem.

cheers,
dan

@Dan: TNX for your time and efforts spent for this issue too, appreciated !
@Andrew: if you agree, could you close the peer review and send it to integration? TIA.

Matteo

Thank you for your time!

Sending back for integration after much (more) testing

Dan Poltawski FYI it was surprisingly easy to compile mod_xsendfile for Ma. The only issue I encountered is that httpd on OS X (Mountain Lion at least) uses a path to cc which doesn't exist on a normal install. I had to symlink /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.8.xctoolchain to /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/ before I could run the apxs -c command.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Hi Dan,
I'm in difficulty with git vs process.
Your rebase request is part of the process (almost an automatic message) but in this issue things have been already integrated and then reverted (as per the discussion above) so rebasing can do nothing.

Here two reverts (reverting the reverts ) are IMHO required to integrate the issue again:

commit a151eb403eae727634050f58f389183eefca4630
Author: Dan Poltawski <dan@moodle.com>
Date:   Thu Jun 13 13:56:11 2013 +0800

    Revert "MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1"

    This reverts commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59.

commit db74804ff1245df4bdc0c8de187cfbe43a3ec0d7
Author: Dan Poltawski <dan@moodle.com>
Date:   Thu Jun 13 13:56:09 2013 +0800

    Revert "MDL-39832 Files: Fixed ETag format according with RFC2616."

    This reverts commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b.

commit 0a006517e5620f32e8a22fddcb43103992e5b134
Merge: f4c824c 9ab37ae
Author: Dan Poltawski <dan@moodle.com>
Date:   Mon Jun 10 15:42:45 2013 +0800

    Merge branch 'm26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile' of https://github.com/scara/moodle

commit 9ab37ae3e07f54bc08644f8462d94bed68ff6c9b
Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com>
Date:   Sat Jun 1 12:34:40 2013 +0200

    MDL-39832 Files: Fixed ETag format according with RFC2616.

    ETag must be double quoted,
    http://tools.ietf.org/html/rfc2616#section-3.11:

          entity-tag = [ weak ] opaque-tag
          weak       = "W/"
          opaque-tag = quoted-string

commit 3960c00e3cf129c60b01fecdfe2851ba9ef7ba59
Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com>
Date:   Sat Jun 1 12:21:29 2013 +0200

    MDL-39832 Files: Revert MDL-39688 commit, f111746140fe80a821ae69e3bf518d950bf188a1

    MDL-39688 has been successful in providing a solution for the issue but further
    investigations done in MDL-39832 have got the evidence that the given solution
    is more likely a workaround.

I doubt if I need to push almost a new branch (e.g. m26_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile_Reverting), to apply the correct reverting: any advice will be greatly appreciated.

TIA,
Matteo

Hi Matteo,

I think you should ignore the reverts performed before, just imagine they never happened. Just provide clean branches with the changes you want to be applied to every current upstream branch. I must confess I'm a bit disoriented about what is remaining to be integrated here, please clarify.

Also, I'm keeping this on-hold for this week because practically everybody is out @ the AU Moot these days and it seems that the most important part of this issue is to test/verify nothing is borked. And there is already people experimented to test this that surely will perform the task far better than Sam or me (the only two available this week).

Thanks for the big effort, ciao

Hi Eloy,
taking 2.5 (m25_MDL-39832_Fix_Chrome_Issues_ETag_XSendfile) as an example, here are the two commits to be applied for this issue, MDL-39832:

1. https://github.com/scara/moodle/commit/d618e3c07d36272809b84d72df51067f1daa51c7: reverting the work done by Petr in MDL-39688. It works but it removed the possibility to nicely cache, browser side, the Partial Responses (HTTP 206)
2. https://github.com/scara/moodle/commit/52286ac358bbd81e8291cf0a85054ba63603732e: adding the missing double quotes as it has been identified to be the proper fix for MDL-39688.

If you look at the 2.6 branch you'll see that the two commits have been already applied from my branch and then reverted due to the bad testing result, which has been found to be inconsistent in the next integration round.

Shortly, for 2.3, 2.4, 2.5 you already have the branches (I'll rebase them) while for 2.6 we need to decide if:
a. you revert the two reverts done by Dan w/ no need for rebasing on my side
b. I re-create the two commits reverting Dan's reverts, in a new branch since my proposal for 2.6 has been already applied into master
c. I apply the required fix, maybe not with the same commits history used in the other branches, in a new branch without explicitly reverting Dan's reverts

The goal should IMHO be how to keep the history consistent between branches and I'm wondering if (a) could be the right option.

Hi Eloy,
back again, I've made a mistake in my previous comment; all the branches have been already "fixed" and "reverted", e.g.:

• https://github.com/moodle/moodle/commits/master/lib/filelib.php
• https://github.com/moodle/moodle/commits/MOODLE_25_STABLE/lib/filelib.php
• https://github.com/moodle/moodle/commits/MOODLE_24_STABLE/lib/filelib.php
• https://github.com/moodle/moodle/commits/MOODLE_23_STABLE/lib/filelib.php

lol, I'm getting crazy with this, hahaha.

I don't mind what has happened until now in any of the branches. If something was sent and later reverted it 100% the same than assuming it never landed in first term.

So, right now, the master branch is into A status and you want to move it to B status, so all we need is a commit performing the changes from A to B.

I really don't get the point about reverting the reverts not what has been partially or totally added modified by other issue. Just add as many commits you need to go successfully to B from current status. if for master you need 2 or more commits, one undoing what was done in another issue and another for the fixes originally related to this issue, np here at all.

But I think we must not be introducing re-re-re-reverts. Just imagine it never happened, really.

If that is alternative (c) in your list above... then go for it, IMO. Erase those 2 commits from your mind (the original and its revert, they sum, together, 0 changes).

Not sure if I'm missing something or no... sorry, thanks and ciao

OK,
will do all the required changes to move from now (A) to what required (B).

Matteo

Many thanks Matteo!

I'm sending this back to development, so will be picked next week (or when available, no pressure!).

Ciao

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

For the/my record, interesting reading: https://www.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.txt

Hi Andrew,
would you be so kind and patient to perform another review and propose the fix back again to the integration review?

TIA!
Matteo

(Sorry I didn't see this message earlier Matteo).

Basically, you need to re-apply the commits, which is kind of ugly I agree - and rebase doesn't do the right thing, because it thinks its already been commited.

NP Dan!
Already done and learned something new which is fine .

Rebased, just in case it will be integrated the next week .

Thanks Matteo, those commits look pretty clear now!

Peer review looks good to me still (no change). Submitting for integration now.

Rebased.

Thanks guys, this has been integrated in 2.3, 2.4, 2.5 and master.
Dan volunteered to be a tester

Hurray, passed. Thanks Matteo.

I noticed a few things while testing:

1. The quicktime plugin seems screwed on my machine on first load. It seems to work ok one cached. This was unrelated to this patch and also to xsendfile on or off. I think this was the problem I saw when testing earlier.
2. We seem to be serving a YUI file mistakenly using x-sendfile:

   (20023)The given path was above the root path: xsendfile: unable to find file: /Users/danp/git/integration/lib/yuilib/3.9.1/build/assets/skins/sam/sprite_icons.png, referer: https://dan.moodle.local/integration/mod/forum/post.php?forum=1
   

   However, I couldn't reproduce this when I tried agian.

ps. Thanks Andrew for the xsendfile note. It was suprisingly easy and that symlink tip helped me avoid messing around to fix it.

Regarding with the xsendfile error, let me share some of my notes: XSendfilePath should be defined e.g. as /Users/danp/git, without the trailing slash and using forward slashes for both *nix and Windows... if everything worked as expected there was no issue with AllowOverride FileInfo too and I've no clear reasons for that error in your log file.

Surely not - are we not restricting xsendfile served files to moodledata? I didn't think it was a solution for serving dirroot files.

Good point!
This morning I didn't explicitly focus on being an error related to what should be supposed to be served as a static file, being under dirroot because I'm supposing this behaviour to be by-design: those IMG files are referenced in CSS files as relative to the CSS themselves, the CSS files being served through a PHP helper - I know you know it , just for the record - for managing also the theme versioning and performing other required stuff like rewriting the path of the imgs too (ref.: yui_compo.php) i.e. that is the reason why X-Sendfile is running even on dirroot, raw details: theme/yui_image.php.

a single bug fix
a drop in a waterfall
hear the mighty roar

Thanks for your contribution!

Just to comment that his causes a regression on etag verifications that are being commented @ https://tracker.moodle.org/browse/MDL-38743?focusedCommentId=250106&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-250106

Ciao