564387 - "Save Video As..." does not respect the filename set in the Content-Disposition header

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Mathieu Pellerin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100506 Minefield/3.7a5pre
Build Identifier: 

A web application I'm building is offering ogg theora videos using the <video> tag. The video's source is a PHP script that outputs the .ogv video. The tag looks like this:

<video src="outputvideo.php?id=1">

In the PHP script, a Content-Disposition header is set to give a proper filename if/when users decide to save the video:

header('Content-Disposition: filename="video-'.$id.'.ogv"');

In both Chromium and Opera, the "save video as..." link will respect the filename set in the header. Firefox however does not.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

This should work, unless the server takes a long time to respond with the data.  Can you point me to a page where it doesn't work?

Comment 2

[Mathieu Pellerin]

Boris, let me try to get relevant bits on a public website for you to test. As I said, it currently works with chromium & opera.

The response time is very quick as the web app is running on an intranet.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  It looks like the UI code here is using the wrong way to save, which in fact won't pick up content-disposition.  Sorry for the trouble of asking for a page showing this.  :(

Not a core issue, though.

In particular, looks like nsContextMenu is using saveURL directly instead of either doing what "save link as" does or grabbing the content-disposition from the object like saveImageURL does.

Comment 4

[Kailas]

Boris, It looks like there is a need of an interface like imgICache for Video/Audio.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why?  I don't think anything like that is needed.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think "Save Video As..." needs to initiate a new HTTP connection, so it should get its own Content-Disposition header and work like "Save Link As...".

Comment 7

[Kailas]

Sending new HTTP request will work, but I feel its extra overhead of sending HTTP request and waiting for its response. Instead, isn't it possible to retrieve Content-disposition header information from the nsIMultiPartChannel, if we get reference to it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

What nsIMultiPartChannel?  There isn't one.  There may be no necko channel around at all when video is being played.  More importantly, unlike images, the video may well not be cached in its entirety.

Comment 9

[Kailas]

Boris, Sorry, my mistake. I was looking into nsIMIMEHeaderPara, to retrieve the information if we get reference to it.
Thanks for your clarification.

Comment 10

[Kailas]

Attachment: Patch to wait for appropriate MIME headers and then prompt the user with a file picker.

Patch to get MIME-type headers.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That seems to be duplicating a bunch of the "save link as" code, right?  Can the code not be shared?

Comment 12

[Kailas]

Boris: Yes, Its a lot of duplication of code. I will create another patch with code sharing.

Comment 13

[Kailas]

Attachment: updated Patch on changeset: 63446:8618a149ea2e

Updated Patch

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 520632 [details] [diff] [review]
updated Patch on changeset: 63446:8618a149ea2e

Looks ok to me, but this isn't code I'm very familiar with. Gavin is probably a better reviewer for this than I am, if not, he'll know who is.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 520632 [details] [diff] [review]
updated Patch on changeset: 63446:8618a149ea2e

The general approach looks good, thanks for the patch! A few preliminary comments:

- you seem to be removing some useful comments when factoring out the code - you should revert that aspect, I think. Also the function naming changes (e.g. saveMediaAs_onStartRequest) aren't necessary.
- you're adding tabs for whitespace, which should be avoided (keep the indentation the same)
- Using mercurial with the diff options from https://developer.mozilla.org/en/Installing_Mercurial#Configuration to generate your patches is recommended (short of that, just passing -u8 to |diff| would help too)

All of those should make the resulting diff much easier to read, so if you could resubmit a patch with those addressed and re-request review, that'd be great.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

This is also a piece of code that we seem to not have any automated test coverage for, so at the very least it would be useful if you could mention what kind of manual testing you've done to ensure both primary UI features affected by the patch still work as expected.

Including an automated test would be even better, but that might be tricky :)

Comment 17

[Kailas]

Gavin: Thanks for your valuable feedback. I will update the patch and submit it. And also provide the information about manual testing I did.

Comment 18

[Kailas]

Attachment: Updated Patch

Updated Patch against  changeset:   63865:e11c2f95f781

Made changes according to Comment 15.

Comment 19

[Kailas]

To ensure both primary UI features affected by the patch works as expected, I did manual testing as follows:

1. I configure VM running apache web server.
I stored video-1.ogv, video-2.ogv and audioFile.mp3 media files on the webserver.

I created bug564387.html file as follows: 

<html>
<body>
Testing for Mozilla Bug: 564387<br>
<video src="contentheader.php"> </video>
<br>
<video src="video-2.ogv"> </video>
<br>
<audio src="audioFile.mp3" controls="controls" autoplay="autoplay">
 Your browser does not support audio element
</audio>
</body>
</html>

The contents of "contentheader.php" file is as follows:

<?php
$file = 'video-1.ogv';
if(file_exists($file)){
	header('Content-type: video/ogg');
	header('Content-Disposition: attachment; filename="video-1.ogv"');
	header('Content-Length: ' . filesize($file));
	header('Content-Transfer-Encoding: binary');
	ob_clean();
	flush();
	readfile($file);
	exit;
}
?>

2. I open bug564387.html file from my host machine. Right click on first embedded video in the web page and selected "Save Video As.." context menu option. It displayed file picker with the correct file name. That is "video-1.ogv". Then I also tested for second embedded video in the web page which does not use content-disposition header. It also displayed me correct file name in the file picker window, that is "video-2.ogv" 
I also observed similar result for audio file. 

3. To test "Save Link As.." option in context menu, I randomly searched on Google and right click on links and selected "Save Link As.." context menu option. It displayed correct file name in file picker.

Please correct me If I am mistaken.

Thanks,
Kailas

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Thanks for the followup, Kailas - great work!

A couple of further thoughts here:
- This seems to change the behavior for "save video" such that we now avoid using the cache. Does that have any effect in practice? If so, do we want that? I'm not sure what the reasoning behind bypassing the cache for "save link" is (we don't bypass it for "Save image as").
- I was wrong about this code path not being tested, we have some "Save as..." tests in toolkit/content/tests/browser/. This might be one of the more complicated cases to test, but perhaps Paolo would be willing to help?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what the reasoning behind bypassing the cache for "save link" is

For what it's worth, I tracked back the blame for this.  It dates back to bug 120174.  Unfortunately, the comments in that bug indicate _what_ the patch does, but not _why_.  Ask Ben and see whether he remembers?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #21)
> For what it's worth, I tracked back the blame for this.  It dates back to bug
> 120174.  Unfortunately, the comments in that bug indicate _what_ the patch
> does, but not _why_.  Ask Ben and see whether he remembers?

Yes, I went through exactly the same frustrating process :) I'm sure he doesn't remember.  Mossop's theory was that perhaps it was decided that bypassing the cache in general was best (to avoid saving stale content), but that for images on the page itself, it's more important to try and ensure you're saving what you're seeing (a concern that doesn't apply with "save link as").

Extending that argument, it seems like we'd want to not bypass the cache for video. Except that I'm not sure whether the BYPASS_CACHE load flag has an effect for video at all, given that I know they are cached differently.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In practice videos almost never get cached by Necko, so this won't matter in practice. But it sounds like not using BYPASS_CACHE is the right thing in principle.

Comment 24

[:Paolo Amadini]

(In reply to comment #20)
> - I was wrong about this code path not being tested, we have some "Save as..."
> tests in toolkit/content/tests/browser/. This might be one of the more
> complicated cases to test, but perhaps Paolo would be willing to help?

Sure! Even if now I can't help with writing the test itself, I can provide a
few pointers and guidance. Kailas, if you want to write the test, that would
be really appreciated, and it will help getting the patch in the product
sooner. You should be armed with patience, nonetheless :-)

I think the steps in comment 19 are a very good way to test the code affected
by the patch, they just need to be automated, so that they're executed when
you run the following command in your build environment (assuming you have
one with tests enabled):

TEST_PATH=toolkit/content/tests/browser make mochitest-browser-chrome

The first step is to make your test page and the videos available to the test
code. A minimal integrated web server is automatically started by the test
framework, so you just have to put your files inside the
"toolkit/content/tests/browser/data" folder, edit "Makefile.in" there,
and they're available at this local address while the tests are running:

http://mochi.test:8888/browser/toolkit/content/tests/browser/data/

Note that the files you'll find there are ".sjs" because they need to
respond to POST data. In your case you don't need that, you can just
add a plain ".html", copy one of the other test videos in the tree...

http://mxr.mozilla.org/mozilla-central/find?string=.ogv

...and then add a file named "<your_video>.ogv^headers^" to the directory
and to the makefile, as explained here:

https://developer.mozilla.org/En/Httpd.js/HTTP_server_for_unit_tests#Header_modification_for_files

Finally, to test manually that the above works before writing the automated
test... I don't know the right way :-) I generally edit my tests so that
don't run to completion and leave the browser window open, so that I can
interact with it before the test times out. For example, you could comment
this line in "browser_save_resend_postdata.js":

    //gBrowser.addEventListener("pageshow", pageShown, false);

If you have any question, feel free to ask. Once you can execute the test
manually upon the mochitest framework, we can continue from there.

Gavin, others, is there any better way of keeping the browser open, with the
test profile loaded and the web server up and running?

Comment 25

[Kailas]

paolo: Thanks for detailed explanation. I followed the steps in comment 24 and I can execute the test manually upon the mochitest framework.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #24)
> Gavin, others, is there any better way of keeping the browser open, with the
> test profile loaded and the web server up and running?

You can invoke runtests.py manually, rather than relying on the Make target[1][2], and then just omit --autorun and/or --close-when-done:

> python objdir/_tests/testing/mochitest/runtests.py --browser-chrome --test-path=toolkit/content/tests/browser

[1] http://mxr.mozilla.org/mozilla-central/source/browser/build.mk#90
[2] http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#58

Comment 27

[:Paolo Amadini]

(In reply to comment #25)
> paolo: Thanks for detailed explanation. I followed the steps in comment 24 and
> I can execute the test manually upon the mochitest framework.

Great! Now comes the trickiest part, however most of the steps to be automated
are already implemented in "browser_save_resend_postdata.js". You should
create another test file with the same basic structure. There are probably
multiple ways of invoking the right code path, I'd make the test do the
following steps:

* Load your test page (same method as the existing test).

* Ensure that the window is focused. Something like:
  SimpleTest.waitForFocus(testRunner.continueTest);
  yield;

* Synthesize the right click on the context menu, and wait for it to be shown.
  You can register a listener for "popupshown" on the context menu (same
  method as "pageshow" above), and use "EventUtils.synthesizeMouseAtCenter"
  on your link. For reference:

  http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/test/browser_bug409481.js#45

* Create a temporary save directory, register the mock file picker object
  that will intercept the save dialog, register a mock download progress
  listener to detect when the download terminates, and only then synthesize
  the left click on the context menu item to start the process, and wait
  for the download to complete. This is similar to the current test.

  Note that, since the code in the context menu operates asynchronously,
  we cannot use "callSaveWithMockObjects" as we do with "saveDocument",
  because this would unregister the mock objects too early. You still need
  to do what "callSaveWithMockObjects" does, but directly inside the
  "_TestGenerator" function, ensuring that "var downloadSuccess = yield;"
  and the rest is executed before the "unregister" functions.

* Check if "mockFilePickerResults.selectedFile" (which is an nsIFile) has
  the right leaf name.

This is for one link only. For testing multiple links, you can just add a
"for" loop in the function, for example iterating over pairs of link IDs
in your document and expected file leaf names. Note that you should use
the plain "for" keyword, not Array.forEach or other calls that require a
nested function, since you are inside a generator function that uses the
"yield" statement (same reason for avoiding callSaveWithMockObjects).

I'd add that if you want to do something more, since we don't actually
need to save the video to know if the name was selected correctly, you
could edit the testing framework in the "common" folder so that we can
just check the file name and cancel the save operation. But even just
the procedure above should be more than enough for our case.

When the test works, you can attach a single patch including both your
code and its tests. But, of course, you should still make sure that the
test fails if run without your modifications to the production code.

Comment 28

[:Paolo Amadini]

(In reply to comment #26)
> You can invoke runtests.py manually, rather than relying on the Make
> target[1][2], and then just omit --autorun and/or --close-when-done:
> 
> > python objdir/_tests/testing/mochitest/runtests.py --browser-chrome --test-path=toolkit/content/tests/browser

Thanks for the tip! I'll add this to my build recipes :-)

Comment 29

[Kailas]

Paolo: In TestGenerator I successfully loaded the page and ensure window is focused. I added event listener to "popupshown" but it doesn't go inside popupShown function. 

function videoLoaded()
{
  testRunner.continueTest();
}


function popupShown()
  {
      dump("\n\n\n\n\n\nPopUp is Shown\n\n\n\n");
      gBrowser.removeEventListener("popupshown", popupShown, false);
      var saveVideoCommand = gBrowser.contentDocument.getElementById("context-savevideo");
      saveVideoCommand.addEventListener("command", saveVideoCommandExecuted, false);
      saveVideoCommand.doCommand();
  }

 function saveVideoAs_TestGenerator() {
     
    gBrowser.loadURI(kBaseUrl + "test_bug564387.html");
   
    SimpleTest.waitForFocus(testRunner.continueTest);
    yield;
   
    try {
	gBrowser.addEventListener("DOMContentLoaded",
                                videoLoaded, false);
	yield;
      try {
	  var video1 = gBrowser.contentDocument.getElementById("video1");
	  gBrowser.addEventListener("popupshown", popupShown, false);
	  EventUtils.synthesizeMouseAtCenter(video1, {type: "contextmenu", button: 2 }, gBrowser.contentWindow);
	  
      }
      finally {
        gBrowser.removeEventListener("DOMContentLoaded",
                                     videoLoaded, false);
      }
......

Could you please give some hint?

Comment 30

[:Paolo Amadini]

(In reply to comment #29)
> Paolo: In TestGenerator I successfully loaded the page and ensure window is
> focused. I added event listener to "popupshown" but it doesn't go inside
> popupShown function.

If you see the context menu opening, but you don't get the event, probably
the reason is that you should add the event listener on the XUL element of
the context menu, rather than the browser element. Maybe just adding the
listener to gBrowser.contentDocument instead of gBrowser may suffice.

A couple of notes while I'm looking at this: first, there is no need to have
separate popupShown or videoLoaded functions as you may have seen in other
test files, just have your event listener call "testRunner.continueTest",
and then call "yield" (which blocks until continueTest is actually called),
before going on with the other business in the same function. The resulting
code will be easier to follow from top to bottom.

Second, you should add the event listener for "pageshow" before "loadURI",
and remove it after "yield", otherwise you might incur in a race condition
and your test might intermittently fail, if the event is raised before the
listener is registered. For a similar reason, you should wait for focus on
the window only after you responded to "pageshow".

Note that, in the reference test, the "DOMContentLoaded" event listener is
registered to wait for the sub-page that is loaded by the following "submit"
call. In your case there are no sub-pages, thus "pageshow" is better, also
because it is raised later, when more elements in the page have completed
loading:

https://developer.mozilla.org/en/Gecko-Specific_DOM_Events

Ensure you use "function pageShown(event)" for listening to "pageshow". It
filters spurious events that we might receive because previous tests loaded
the "about:blank" page.

Comment 31

[Kailas]

Paolo: Thanks for your suggestions and detailed explanation. It really helped me.
Adding listener to "gBrowser.contentDocument" doesn't work but adding listener to  "document" worked. 
Now I can see the save file dialog box with correct filename. But I don't 
need to save the video file. Is it ok if I read the file name in text field of the save dialog box and synthesize the click on "Cancel" button of Save file dialog box?

Comment 32

[Kailas]

Attachment: Patch with TestCase

Patch with Testcase
Updated Patch with Testcase against changeset:   68152:29ea31633ac6

Test case created according to comment 27.

Comment 33

[:Paolo Amadini]

Comment on attachment 528275 [details] [diff] [review]
Patch with TestCase

(In reply to comment #31)
> Paolo: Thanks for your suggestions and detailed explanation. It really helped
> me.

Thanks to _you_ for contributing! I see you've figured out what to do in the
last part of the test, to have it run correctly on your machine: good work!

Now the patch has to go through the usual review process, where every little
detail is examined until the end result is prefect :-) I can do a first pass
review on the current version, though in the next iteration you'll have to
use the flag to request review from Gavin, which is more experienced than me
and will notice details (and important things) that I'll probably overlook.

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>-          // before we get credentials from user.  Both authentication dialog
>-          // and save as dialog would appear on the screen as we fall back to
>+          // before we get credentials from user. Both authentication dialog
>+          // and save as dialog would appear on the screen as we fallback to

nit: fixing typos is fine, but I don't think we care changing English-style to French-style spacing, or vice versa. In the current source code, I've seen both spacing styles used after the full stop.

>--- a/toolkit/content/tests/browser/Makefile.in
>+++ b/toolkit/content/tests/browser/Makefile.in
>   browser_Geometry.js \
>   browser_save_resend_postdata.js \
>   browser_Services.js \
>   $(NULL)
>+ 

nit: no need to add this blank line. Changing spacing unnecessarily makes it more difficult to merge changes across branches.

>+++ b/toolkit/content/tests/browser/browser_save_video.js
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

In general, we now usually dedicate test code (as opposed to product code) to the public domain. It saves some boilerplate text :-)

http://www.mozilla.org/MPL/license-policy.html

I'm fine to dedicate my existing Mozilla test code to the public domain, though I've not updated the license boilerplate in all files yet. If you're fine with this too, just use the following header in the new test file:

http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

>+   function pageShown(event)
>+   {
>+     if (event.target.location != "about:blank")
>+       testRunner.continueTest();
>+   }

The correct indentation is two spaces per level. Also check indentation later in this file.

>+      // Synthesize the right click on the context menu, and
>+      // wait for it to be shown
>+      document.addEventListener("popupshown", testRunner.continueTest, false);

You should remove every event listener that you add, otherwise this will affect tests executed later.

>+      EventUtils.synthesizeMouseAtCenter(video1, 
>+                                         {type: "contextmenu", button: 2 },
>+                                         gBrowser.contentWindow);

nit: Space after "{"

>+	    // Select "Save Video As" option from context menu
>+	    // Synthesize left click on the context menu
>+	    var saveVideoCommand = document.getElementById("context-savevideo");	 
>+	    saveVideoCommand.addEventListener("command", 
>+                                              testRunner.continueTest, false);
>+	    saveVideoCommand.doCommand();
>+	    yield;

This doesn't do what's advertised (synthesize left click on the context menu). Calling doCommand might be fine, the difference is that I'm not sure if this closes the context menu. If you interrupt the test after doCommand, is the context menu still open? If yes, you must ensure that it is closed. Just synthesizing the left click will do it, but other methods are also fine.

I also think you don't need to register a listener for "command", at this point waiting for the download to finish is enough, since you don't need to do any other action meanwhile.

>+++ b/toolkit/content/tests/browser/data/Makefile.in
> 
> _DATA_FILES = \
>+  test_bug564387.html \
>+  bug564387_video1.ogv \
>+  bug564387_video1.ogv^headers^ \
>+  mixedContentTest.js \

I guess mixedContentTest.js is just a leftover from local tests.

>diff --git a/toolkit/content/tests/browser/data/bug564387_video1.ogv b/toolkit/content/tests/browser/data/bug564387_video1.ogv
>new file mode 100644
>index 0000000000000000000000000000000000000000..093158432ac64090f3572fa7167da8e102b94515
>GIT binary patch

Please use "hg copy" from the reference file you used in the repository.

>+++ b/toolkit/content/tests/browser/data/bug564387_video1.ogv^headers^
>+Content-Disposition:  filename="Bug564387-expectedName.ogv" 

nit: Only one space after ":"

>+Content-type: video/ogg

nit: Content-Type

Comment 34

[Kailas]

Paolo: Thanks for your suggestions. I will correct those errors and submit the updated copy. I am fine to dedicate it to public domain so I will change the header in the new test file.

Comment 35

[Kailas]

Attachment: Updated Patch with Test Case

Updated Patch with Testcase against changeset:   68152:29ea31633ac6
Errors corrected according to comment 33.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528297 [details] [diff] [review]
Updated Patch with Test Case

This really is excellent work - I apologize again for the delay here. This patch looks great, but there's one minor change we should make: saveHelper should take an aBypassCache parameter, which controls whether it passes LOAD_BYPASS_CACHE in the common case, and the aShouldBypassCache parameter it passes to saveURL for the fallback case. It should be false for the video case, and true for the "save link as" case, to match current behavior.

Comment 37

[Kailas]

Attachment: Updated Patch: changeset: 68152:29ea31633ac6

Updated Patch.
changeset: 68152:29ea31633ac6

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 531249 [details] [diff] [review]
Updated Patch: changeset: 68152:29ea31633ac6

Close, but not quite :) There should be one flag: aBypassCache. It should be passed directly to saveURL, and also control whether we pass the Ci.nsIRequest.LOAD_BYPASS_CACHE flag, e.g.:

let flags = Ci.nsIChannel.LOAD_CALL_CONTENT_SNIFFERS;
if (aBypassCache) 
  flags |= Ci.nsIRequest.LOAD_BYPASS_CACHE;
channel.loadFlags |= flags;

Comment 39

[Kailas]

Attachment: Updated Patch: changeset: 68152:29ea31633ac6

updated patch: Automated testing pass but manual testing failed.

Comment 40

[Kailas]

Gavin: Example to test is available online on following url:
http://kailas.scienceontheweb.net/content.html

Expected result: 
1. Video 1: Right-click on Video 1 and select "Save As video" from context menu should prompt to save a file with name "video-1.ogv"
2. Video 2: Right-click on Video 1 and select "Save As video" from context menu should prompt to save a file with name "video-2.ogv"


Another observation: Second video in the above webpage is not properly loaded in FF but I tested above example with Google Chrome(11.0.696.65 (84435) Ubuntu 10.04) and it works properly with Google Chrome. Perhaps, this behavior is not related to this bug. This might be an another bug.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're serving video-2.ogv as "text/plain", which is not a valid video format.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Attachment: working nsHttp:5 log

I looked into this again just recently. I confirmed the issue mentioned in comment 40. For some reason, passing LOAD_BYPASS_CACHE causes the video download to fail. Here's an nsHttp:5 log of the working transfer.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Attachment: broken nsHttp:5 log

... and here's a log of the broken download. Progress seems to just stall after OpenCacheEntry - in the working case that's followed by a call to OnCacheEntryAvailable(), but in the broken case that doesn't happen, and  Connect() never gets called, and the transfer is left pending (eventually the timer fires and calls Cancel() on the channel, but since it hasn't started yet that has no effect, so onStopRequest also isn't fired, so we don't ever fall back to the other download path).

bz, roc, do you guys have any idea what might be going on here? Why would passing LOAD_BYPASS_CACHE cause a load of a video file to fail? Seems like it's potentially related to the special media cache setup, but I don't know anything about how that works.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I wouldn't have thought so. You're not actually passing LOAD_BYPASS_CACHE through any code path that the media elements use (are you?).

Comment 45

[:Gavin Sharp [email: [e-mail]]]

No, I'm not - this is custom download code used only by the context menu. Fairly standard nsIChannel use, AFAICT. I guess I don't actually have evidence to suggest that this is specific to video elements - I'll see if it affects other cases too.

Comment 46

[Boris Zbarsky [:bzbarsky]]

LOAD_BYPASS_CACHE means "don't try to read it from the cache".  But necko will still to _write_ it into the cache, of course.

Now our cache can only handle one consumer per cache entry at a time.  In your case, presumably the video is still playing, so the cache entry is held open by the channel started by the <video> tag.  It's probably even writing it to the cache.  When you start a new network requests that wants to write to the same cache entry, it has to wait for the cache entry to be released so the writes won't race.  I will bet that's what you're seeing.

The simplest solution here is probably to also use the nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag, which will just skip all interaction with the cache entirely if it can't immediately get a cache entry.

And yes, this is not specific to video.  See the bit in nsXMLHttpRequest::Send that adds that flag for sync XHR.  ;)

Comment 47

[Kailas]

Attachment: Working patch: changeset:72611:8753de11b181

updated patch: Both Automated testing and Manual testing passed.

Boris: Thanks for your suggestion.

Comment 48

[Boris Zbarsky [:bzbarsky]]

You should probably only use the nsICachingChannel flag for channels that implement that interface.  It's in the per-channel flag area, so other channels could use that flag to use something totally different.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

bz: thanks for figuring that out.

Kailas: Seems to me like you should be passing LOAD_BYPASS_LOCAL_CACHE_IF_BUSY unconditionally (but only if channel instanceof nsICachingChannel, as bz suggests).

I don't see how that patch would fix the issue we were seeing, because the video case falls into the aBypassCache=true branch, which you're not setting the new flag in.

Comment 50

[:Gavin Sharp [email: [e-mail]]]

Oh, sorry, I got confused again. We're not using LOAD_BYPASS_CACHE for the video case, but we are for "Save Link As". But that's mostly irrelevant since the LOAD_BYPASS_LOCAL_CACHE_IF_BUSY problem is what matters.

Comment 51

[Kailas]

Attachment: Working patch: changeset:72611:8753de11b181

Comment 52

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Thanks again Kailas, this is great work. I addressed a few minor nits (parameter name, spacing, trailing whitespace, etc.), and moved the test from toolkit/ to browser/ since it depends on specifics of the Firefox context menu. This involved changes to the toolkit test files to avoid them being dependent on the test location - flagging Paolo to review those. Otherwise this should be ready to land!

Comment 53

[:Paolo Amadini]

Comment on attachment 546234 [details] [diff] [review]
patch

(In reply to comment #52)
> This involved changes to the toolkit test files to avoid them
> being dependent on the test location - flagging Paolo to review those.
> Otherwise this should be ready to land!

Looks good to me :-)

Comment 54

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b93bda5c988c

Comment 55

[Dão Gottwald [:dao]]

Comment on attachment 546234 [details] [diff] [review]
patch

>--- /dev/null
>+++ b/browser/base/content/test/browser_save_video.js

>+            // Close the context menu
>+            document.getElementById("placesContext").hidePopup();

Wrong id, apparently. I'm fixing this in bug 672090.

Comment 56

[Mathieu Pellerin]

Being the initial reporter of this bug, simply want to add a thanks to all involved in fixing it.

Comment 57

[u279076]

Not sure if this is 100% verified by what I did -- can someone please review my testing?

Steps:
1) Download http://hg.mozilla.org/mozilla-central/file/b93bda5c988c/browser/base/content/test/bug564387.html to a local test folder
2) Download http://hg.mozilla.org/mozilla-central/file/b93bda5c988c/browser/base/content/test/bug564387_video1.ogv to the same local test folder
3) Drag bug564387.html to a new tab in Firefox
4) Right click the video, select "Save Video As"

Result:
Video saved as "bug564387_video1.ogv"

Tested using Firefox 8.0b1 and 9.0a2.

Comment 58

[Kailas]

ashughes: If Content-Disposition header is absent then file will be save with its name. It's expected behavior. You also need to use bug564387_video1.ogv^headers^ file on your test server.

Comment 59

[Vlad [QA]]

Could not verify without a web server.
Anthony can I change the whiteboard to "qa?" or "qa-"?
Thanks