Closed
Bug 564387
Opened 14 years ago
Closed 13 years ago
"Save Video As..." does not respect the filename set in the Content-Disposition header
Categories
(Firefox :: File Handling, defect)
Tracking
()
RESOLVED
FIXED
Firefox 8
People
(Reporter: nirvn.asia, Assigned: patilkr24)
References
Details
(Whiteboard: [qa+][testday-20110930])
Attachments
(1 file, 11 obsolete files)
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•14 years ago
|
||
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?
Reporter | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Component: Video/Audio → File Handling
Ever confirmed: true
Product: Core → Firefox
QA Contact: video.audio → file.handling
Boris, It looks like there is a need of an interface like imgICache for Video/Audio.
Comment 5•13 years ago
|
||
Why? I don't think anything like that is needed.
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...".
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•13 years ago
|
||
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.
Boris, Sorry, my mistake. I was looking into nsIMIMEHeaderPara, to retrieve the information if we get reference to it. Thanks for your clarification.
Assignee | ||
Comment 10•13 years ago
|
||
Patch to get MIME-type headers.
Attachment #520604 -
Flags: review?
Attachment #520604 -
Flags: review? → review?(jst)
Comment 11•13 years ago
|
||
That seems to be duplicating a bunch of the "save link as" code, right? Can the code not be shared?
Assignee | ||
Comment 12•13 years ago
|
||
Boris: Yes, Its a lot of duplication of code. I will create another patch with code sharing.
Assignee | ||
Comment 13•13 years ago
|
||
Updated Patch
Attachment #520604 -
Attachment is obsolete: true
Attachment #520632 -
Flags: review?
Attachment #520604 -
Flags: review?(jst)
Attachment #520632 -
Flags: review? → review?(jst)
Comment 14•13 years ago
|
||
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.
Attachment #520632 -
Flags: review?(jst) → review?(gavin.sharp)
Comment 15•13 years ago
|
||
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.
Attachment #520632 -
Flags: review?(gavin.sharp) → review-
Updated•13 years ago
|
Assignee: nobody → patilkr24
Comment 16•13 years ago
|
||
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 :)
Assignee | ||
Comment 17•13 years ago
|
||
Gavin: Thanks for your valuable feedback. I will update the patch and submit it. And also provide the information about manual testing I did.
Assignee | ||
Comment 18•13 years ago
|
||
Updated Patch against changeset: 63865:e11c2f95f781 Made changes according to Comment 15.
Attachment #520632 -
Attachment is obsolete: true
Attachment #522056 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 19•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
> 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•13 years ago
|
||
(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.
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•13 years ago
|
||
(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?
Assignee | ||
Comment 25•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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 :-)
Assignee | ||
Comment 29•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 31•13 years ago
|
||
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?
Assignee | ||
Comment 32•13 years ago
|
||
Patch with Testcase Updated Patch with Testcase against changeset: 68152:29ea31633ac6 Test case created according to comment 27.
Attachment #522056 -
Attachment is obsolete: true
Attachment #522056 -
Flags: review?(gavin.sharp)
Comment 33•13 years ago
|
||
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
Assignee | ||
Comment 34•13 years ago
|
||
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.
Assignee | ||
Comment 35•13 years ago
|
||
Updated Patch with Testcase against changeset: 68152:29ea31633ac6 Errors corrected according to comment 33.
Attachment #528275 -
Attachment is obsolete: true
Attachment #528297 -
Flags: review?(gavin.sharp)
Comment 36•13 years ago
|
||
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.
Attachment #528297 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 37•13 years ago
|
||
Updated Patch. changeset: 68152:29ea31633ac6
Attachment #528297 -
Attachment is obsolete: true
Attachment #531249 -
Flags: review?(gavin.sharp)
Comment 38•13 years ago
|
||
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;
Attachment #531249 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 39•13 years ago
|
||
updated patch: Automated testing pass but manual testing failed.
Attachment #531249 -
Attachment is obsolete: true
Attachment #531671 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 40•13 years ago
|
||
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.
You're serving video-2.ogv as "text/plain", which is not a valid video format.
Comment 42•13 years ago
|
||
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•13 years ago
|
||
... 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.
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•13 years ago
|
||
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•13 years ago
|
||
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. ;)
Assignee | ||
Comment 47•13 years ago
|
||
updated patch: Both Automated testing and Manual testing passed. Boris: Thanks for your suggestion.
Attachment #531671 -
Attachment is obsolete: true
Attachment #531671 -
Flags: review?(gavin.sharp)
Attachment #545299 -
Flags: review?(gavin.sharp)
Comment 48•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 51•13 years ago
|
||
Attachment #545299 -
Attachment is obsolete: true
Attachment #545299 -
Flags: review?(gavin.sharp)
Attachment #545570 -
Flags: review?(gavin.sharp)
Comment 52•13 years ago
|
||
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!
Attachment #545011 -
Attachment is obsolete: true
Attachment #545012 -
Attachment is obsolete: true
Attachment #545570 -
Attachment is obsolete: true
Attachment #546234 -
Flags: review+
Attachment #546234 -
Flags: feedback?(paolo.mozmail)
Attachment #545570 -
Flags: review?(gavin.sharp)
Comment 53•13 years ago
|
||
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 :-)
Attachment #546234 -
Flags: feedback?(paolo.mozmail)
Attachment #546234 -
Flags: feedback+
Attachment #546234 -
Flags: checkin?
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: author is Kailas]
Comment 54•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b93bda5c988c
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: author is Kailas]
Target Milestone: --- → Firefox 8
Comment 55•13 years ago
|
||
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.
Reporter | ||
Comment 56•13 years ago
|
||
Being the initial reporter of this bug, simply want to add a thanks to all involved in fixing it.
Updated•13 years ago
|
Attachment #546234 -
Flags: checkin? → checkin+
Updated•13 years ago
|
Whiteboard: [qa+]
Comment 57•13 years ago
|
||
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.
Whiteboard: [qa+] → [qa+][testday-20110930]
Assignee | ||
Comment 58•13 years ago
|
||
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•13 years ago
|
||
Could not verify without a web server. Anthony can I change the whiteboard to "qa?" or "qa-"? Thanks
You need to log in
before you can comment on or make changes to this bug.
Description
•