Closed Bug 240068 Opened 21 years ago Closed 20 years ago

Clicking on some real audio links causes browser to send realplayer an empty temp file

Categories

(Firefox :: File Handling, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: spiritraveller, Assigned: bugs)

References

()

Details

(Keywords: fixed-aviary1.0, regression, Whiteboard: [have patch] fixed?)

Attachments

(2 files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040227 Firefox/0.8 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040227 Firefox/0.8 At the website above, clicking on "Listen Live" causes the RealPlayer (or the trplayer program if that is used) to start up and immediately give an "Invaild Metafile" error. Another user at this <a href="http://www.linuxquestions.org/questions/showthread.php?s=&forumid=2&threadid=165143">page</a> described the exact same problem. Apparently this is caused by the browser downloading a file (e.g. /tmp/g3ntzb74.ra) from the linked url and handing it to the realplayer program. However, when either realplayer or trplayer are started and manually given the <a href="http://play.rbn.com/?url=airam/airam/live/live.rm&proto=rtsp">linked url</a>, they can play the stream with no problems. This does not happen with all realaudio links. Reproducible: Always Steps to Reproduce: 1.Click on the "Listen Live" link at airamericaradio.com 2. 3. Actual Results: Got a dialog box that said "Invalid Metafile" and no audio. Expected Results: I think that firefox should have hand the full url to realplayer instead of downloading an empty file to the /tmp directory and handing it that.
WFM firefox 20040409 windows trunk
(In reply to comment #1) > WFM firefox 20040409 windows trunk Yes, it works on my Windows system running Firefox 0.8. It appears to be a problem only on Linux.
I have verified this: See the link below: http://play.rbn.com/?url=demnow/demnow/demand/2004/april/256/dnB20040430a.rm&proto=rtsp If I use xine or any other media player that handles real format files, the same thing happens: empty file gets passed to the player. www.stileproject.com. Big streaming porno site also causes firefox .08 to choke. Yes even the Apr 30 nightly -- Linux Firefox .07 did not behave this way, all worked fine with firefox 0.7. I can always weasel my way around the bug, but It would be nice if it worked as it did. thanks for reading.
Moving to new File Handling component.
Component: Download Manager → File Handling
Is this still a problem with Firefox 0.9? Please test using a clean profile if it still occurs. Thanks.
(In reply to comment #5) > Is this still a problem with Firefox 0.9? Please test using a clean profile if > it still occurs. Thanks. I created a special user on my box with a new profile, clean profile, and all is the same as it was. the file /tmp/raNdOMnUMber# is empty. Should it not be at least an url? Something is truncating the file. This didn't happen in 0.7 Oh well I can live with a copy & paste of the correct url but I wonder what changed? I wouldn't know where to look in the code, but If I had to embarress my self with an ignorant guess: could it be something to do with a buffer size being suddenly too small for the filename and or the actual stream overwriting the buffer containing the url? As I said it's just an ignorant guess. cheers! Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040615 Firefox/0.9
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1 CONFIRM: I can confirm this bug for Linux Firefox 0.9.1; the bug arises when trying to launch Linux Realplayer 10 beta within Firefox. For example, trying to access the trailers at the Real Movie Guide will trigger this bug: http://www.realguide.com/ Also this bug is specific to Firefox, Mozilla Seamonkey is NOT affected at all. The trailers are playing fine when accessing www.realguide.com with Mozilla 1.6/1.7. Attached is a screenshot showing the "Invalid Metafile error" reported by Realplayer 10 beta on Linux. Mozilla Seamonkey does not trigger this error.
In addition, I think this bug should be considered a block for Firefox 1.0.
What all the problem URLs mentioned have in common is the lack of a filename. The handling of a non-existant filename is what seems to be the problem on Linux. For instance, http://play.rbn.com/?url=airam/airam/live/live.rm&proto=rtsp gives the invalid metafile error, but if I simply add a dummy filename, e.g. http://play.rbn.com/foo.html?url=airam/airam/live/live.rm&proto=rtsp the problem goes away.
BTW, Realplayer 10 Final is released for Linux today. http://www.real.com/linux/ https://player.helixcommunity.org/2004/downloads/ So I am afraid more Linux Firefox users will bump into this bug. Can someone take a look at it?
I can confirm this with RP8 as well / Firefox 0.9.1, which would seem to support the idea that what's getting passed to the player from the browser is boogered up.
I am one of those people mentioned in comment 10. I just installed RealPlayer10 and have not been able to play any of the trailers from http://movies.guide.real.com/ . I'm seeing the exact same error message posted in attachment 153252 [details]. The real bummer of this is all the trailers on Real's site are accessible from the Flash plugin which doesn't expose the actual URL to me. So I can't even cut-n-paste a URL into the RealPlayer. :( I'm curious why this bug is labeled as "minor" and "UNCONFIRMED". Aren't there enough confirmations in here to change that? BTW, I'm using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1.
I can also confirm the exact same problem on Suse Linux 9.1 with firefox 0.9.1
Severity: minor → blocker
I can confirm it with Linux and Firefox 0.9.1, too. It's a major blocker for me, too!
So this file plays fine, assuming they aren't having server problems: http://badloc.pmc.purdue.edu/wbaa/wbaa-am.ram It looks like we can narrow the issue down to links that aren't static, based on this... I do have problems with a link that looks like: http://play.rbn.com/?url=demnow/demnow/demand/2004/aug/audio/dn20040820.ra&proto=rtsp This appears to be some sort of relay or something, but I have no clue how exactly that works. If the above URL expects the browser to follow that link and pass info to RealPlayer, then firefox isn't completing that transaction properly. Interestingly, if you paste the above link into RealPlayer's "Open Location..." box, it works fine. (note that the above link is going to expire in 16-20 hours... sorry). Just thought I'd add this new discovery to the bug.
Flags: blocking-aviary1.0?
1.0PR is also affected.
Mozilla Firefox Developers, please following HelixCommunity Bugzilla Report: https://bugs.helixcommunity.org/long_list.cgi?buglist=3084 Thank you. Thomas Chung FedoraNEWS.ORG (http://fedoranews.org)
If I comment out the following code in nsDownloadManager.cpp, guide.real.com works properly in firefox 1.0PR: // XXXben - // This is not really ideal. If the download is to be handled by a // helper application, then we want to see if there is a duplicate file // in place in the temp folder and remove it _NOW_ before the External // Helper App Service gets a chance to make a unique clone. If we don't, // the EHAS will create a unique version of the name which will muck // with our RDF datasource. We can't create a unique name here either, // because the EHAS isn't smart enough to know that we're fooling with // it... nsMIMEInfoHandleAction action = nsIMIMEInfo::saveToDisk; if (aMIMEInfo) { aMIMEInfo->GetPreferredAction(&action); if (action == nsIMIMEInfo::useHelperApp || action == nsIMIMEInfo::useSystemDefault) { PRBool fileExists; targetFile->Exists(&fileExists); if (fileExists) targetFile->Remove(PR_TRUE); } } I think that the key here is that real.com is serving up files such that mSuggestedFileName.IsEmpty() is evaluating to true in nsExternalAppHandler::LaunchWithApplication, and that when this happens, the tempfile filename gets re-used. I think we have two things trying to rename the downloaded temp file to the mSuggestedFileName. The hack above seems to be trying to clean up an earlier rename before EHAS comes along and tries to do it again (creating a second copy). The problem is, in the mSuggestedFileName.IsEmpty(), mSuggestedFileName = tempfile, so we end up killing the tempfile. The attached patch calls it "untitled" instead, which leaves the tempfile unscathed, making the logic here the same as the general case. That's my theory, anyway :) Some notes from me tracing stuff through (for my future reference): OnLinkClickEvent::HandleEvent -> nsHttpChannel::AsyncOpen nsHttpChannel::OnStartRequest -> nsExternalHelperAppService::DoContent File is successfully downloaded at this point. Open with dialog comes up nsExternalAppHandler::LaunchWithApplication In LaunchWithApplication, we're in a situation where mSuggestedFileName.IsEmpty(), meaning mTempFile->GetLeafName(mSuggestedFileName); gets called. fileToUse->Append(mSuggestedFileName); mFinalFileDestination = do_QueryInterface(fileToUse); nsExternalAppHandler::CreateProgressListener -> nsDownloadManager::AddDownload ... which deletes and recreates the downloaded .ram/.rmm file.
Fixes bug 240068 by making mFinalFileDestination /tmp/untitled
Attachment #159549 - Flags: review+
Attachment #159549 - Flags: review+ → review?(bugs)
Is this "patch" supposed to be a fix? How do you implement that? raysr@ispwest.com
It's a one liner fix, correct Ray. Implemented as seen in the patch.
I could still see the failure in http://guide.real.com and click on any real player files (say the one in Your Top 10). This problem happens only if you open the file directly with the helper app (say real player). It doesn't happen when to donwload that to the local disk and play it.
1. you want to localize that text, and it's past time for that so - 2. you need bz/biesi to review this.
I verfied it and works fine on http://guide.real.com. It was my bad sorry about it...
Comment on attachment 159549 [details] [diff] [review] Fixes bug 240068 by making mFinalFileDestination /tmp/untitled -> bz (I hope)
Attachment #159549 - Flags: review?(bugs) → review?(bzbarsky)
Comment on attachment 159549 [details] [diff] [review] Fixes bug 240068 by making mFinalFileDestination /tmp/untitled This is not acceptable, since it puts data generated by the site into a standard directory (hence also known to the site) with a standard name (hence also known to the site). That's an incredible security risk. This is why we continue to use the randomly-salted temp name in this case right now -- this way the site has a much harder time executing an attack.
Attachment #159549 - Flags: review?(bzbarsky) → review-
Comment on attachment 159549 [details] [diff] [review] Fixes bug 240068 by making mFinalFileDestination /tmp/untitled On the other hand, I suppose we already let the site set the filename (say with Content-Disposition headers).... I guess this change is OK, then, though I think the code quoted in comment 18 is very wrong (it'll blow away random temp files, possibly not even belonging to the Firefox process; and since it passes PR_TRUE to Remove() it will happily blow away entire directory trees). That's pretty unacceptable to me as a user -- it allows a website to effectively delete parts of my temp dir. sr=me assuming the patch uses AssignLiteral() on the trunk, if this is ok with biesi. But I'd really like to hear from Ben on what exactly he thinks EHAS is doing wrong here and how we can work together to enable us to remove that nsDownloadManager code....
Attachment #159549 - Flags: superreview+
Attachment #159549 - Flags: review?(cbiesinger)
Attachment #159549 - Flags: review-
Oh, yes. And Ben's point on localization is a good one. That should happen if we go with this patch.
Can we give this a target milestone of FireFox1.0?
Comment on attachment 159549 [details] [diff] [review] Fixes bug 240068 by making mFinalFileDestination /tmp/untitled this should be AssignLiteral("untitled"); but it really should be localizable. now... why is this better than what we have now? i.e. why isn't the fix to make nsDownloadManager not delete the temp file?
Attachment #159549 - Flags: review?(cbiesinger) → review-
note: I will not give this patch review+ until someone explains why it's needed. note2: the code cited in comment 18 can maybe be made obsolete by implementing nsIObserver and making use of the temp-file topic
I don't understand either, is that why Mozilla Seamonkey on Linux does NOT have this bug, while Firefox has it? What's the difference between them in this File-Handling issue?
>I don't understand either, is that why Mozilla Seamonkey on Linux does NOT have >this bug, while Firefox has it? nsDownloadManager.cpp, cited above, is different on firefox and seamonkey; only the former deletes files.
Re: Comment #33 So using the code of nsDownloadManager.cpp from Mozilla Seamoneky would solve the problem, correct?
Actually, using it would probably make downloading not work at all in firefox; download manager has been pretty heavily forked.
> why is this better than what we have now? It allows the content on guide.real.com to get downloaded correctly, and passed off to the realplayer. Currently, content gets downloaded, deleted, re-created with 0 size, and then passed to the player. This is because guide.real.com is serving up its files with "" as the suggested filename (content-disposition?) We have parties who may be interested in fixing this problem downstream if it doesn't make it into firefox 1.0... beyond being a hack to fix a hack, is there anything technically wrong with the patch? > i.e. why isn't the fix to make nsDownloadManager not delete the temp file? Ben explains why he removes the file in comment 18. I don't fully understand it myself. > note: I will not give this patch review+ until someone explains why it's needed. It's needed to make helpers work correctly when the suggested filename is "" > note2: the code cited in comment 18 can maybe be made obsolete by implementing nsIObserver and making use of the temp-file topic I'd be happy to take a shot at this if someone could go through how all this should be coming together, ie - External Helper App Service (EHAS) registers to listen on the temp-file topic - Download Manager (DM) starts downloading a .rmm file - EHAS gets notified of this temp-file - EHAS signs up with DM to for listening to progress on this temp file - EHAS doesn't create files. DM creates, EHAS listens and executes helper when file is downloaded. I'm probably totally off here, but am looking for something like this... the big picture. Is this something that can make it into Firefox 1.0?
(In reply to comment #36) > It allows the content on guide.real.com to get downloaded correctly, and passed > off to the realplayer. well, ok. that may be factually true. but it doesn't explain why this is not better fixed in ffox code. > This is because guide.real.com is serving up its files with "" as the suggested > filename (content-disposition?) hm, in this case, is this the same as bug 259708? > We have parties who may be interested in fixing this problem downstream if it > doesn't make it into firefox 1.0... beyond being a hack to fix a hack, is there > anything technically wrong with the patch? yes. the fix is at the wrong place. it should be in download manager. > - External Helper App Service (EHAS) registers to listen on the temp-file topic > - Download Manager (DM) starts downloading a .rmm file > - EHAS gets notified of this temp-file > - EHAS signs up with DM to for listening to progress on this temp file > - EHAS doesn't create files. DM creates, EHAS listens and executes helper when > file is downloaded. OK, the basic code flow is described at: http://www.mozilla.org/docs/docshell/mozilla_downloads.png this specific diagram was written with focus on seamonkey but is accurate for firefox as well. Now, after the SetObserver(this) call, ehas tells the download manager about the location of the temp file. Hm, it occurs to me that this may not be sufficient. I'd be happy to introduce a new similar call for the final filename, sometime after/in MoveFile I think. > Is this something that can make it into Firefox 1.0? I don't know 1.0's schedule.
Depends on: 262478
Correcting severity (plugins can't possibly block testing the app). Renominating for 1.0 since Real is popular, giving us a black eye (not to mention other random helper apps which would also be affected).
Severity: blocker → major
Flags: blocking-aviary1.0- → blocking-aviary1.0?
some notes on why this bug happens: files opened with a helper app have a random filename during download. at the end of the download, they are renamed to their "real" name (users demanded this). now a url like from comment 15: >http://play.rbn.com/?url=demnow/demnow/demand/2004/aug/audio/dn20040820.ra&proto=rtsp does not have a filename, and apparently no filename in content-disposition either. so we do not actually rename the file at the end of the download. this means that the nsIFile the downloadmanager code sees is the temporary file (=the final file). which means that if it removes it, the file is gone. this being linux, you can remove files that are still open. Now, nsExternalAppHandler::ExecuteDesiredAction has a CreateUnique call. this obviously creates an empty file here -> that's why we have an empty file, instead of no file. (just thought someone might be interested in that analysis)
Any suggestions as to how this should ideally be fixed? Should the "rename to the file's real name" step be suppressed if the real name is not a valid file name (like an empty string)? ie - the app gets passed the temp file instead of the renamed file? The current fix could be looked at as taking an invalid filename, "", and turning it into "untitled" as part of the renaming process.
(In reply to comment #40) > Any suggestions as to how this should ideally be fixed? Should the "rename to > the file's real name" step be suppressed if the real name is not a valid file > name (like an empty string)? ie - the app gets passed the temp file instead of > the renamed file? that is exactly what happens. sucks that dl manager deleted the file before... > The current fix could be looked at as taking an invalid filename, "", and > turning it into "untitled" as part of the renaming process. but currently, that filename ends up as something like 12345678.ra (NOT as the empty string, due to: - mTempFile->GetLeafName(mSuggestedFileName); ) this must be fixed by not letting dl manager delete files. that is bug 262478 now.
So this bug depends on bug 262478 now. Just wondering, why the bug was introduced in Firefox 0.8 in the first place? Because in Firefox 0.7 it worked fine.
Comment on attachment 159549 [details] [diff] [review] Fixes bug 240068 by making mFinalFileDestination /tmp/untitled Please ignore -- some people here at Real are curious as to how editing patches as comments works in bugzilla.mozilla.org >--- nsExternalHelperAppService.cpp.~1.253.2.1.2.1.~ 2004-09-09 10:43:48.000000000 -0700 >+++ nsExternalHelperAppService.cpp 2004-09-20 21:08:15.000000000 -0700 >@@ -2116,8 +2116,7 @@ My inline comment goes here > if (mSuggestedFileName.IsEmpty()) > { >- // Keep using the leafname of the temp file, since we're just starting a helper >- mTempFile->GetLeafName(mSuggestedFileName); >+ mSuggestedFileName.Assign(NS_LITERAL_STRING("untitled")); Another inline comment > } > > #ifdef XP_WIN
Sorry for the noise guys :) There's a question about how your bugzilla deals with linking to attachments. Here're some examples: attachment 153252 [details] bug 240068 comment 3 paragraph 3
hi ryan, would it be possible for you to use http://landfill.bugzilla.org for your testing? that one has databases specifically set up for testing, contrary to this bugzilla. (another way to link to a comment: bug 240068 comment 44. heh, bugzilla interpreted your last comment as that)
Keywords: regression
Whiteboard: [have patch]
Though the attached patch is -'d, the fix in bug 262478 appears to fix guide.real.com on linux.
so can this bug be closed now?
Whiteboard: [have patch] → [have patch] fixed?
It can be marked fixed IMO.
thanks. fixed by bug 262478.
Status: NEW → RESOLVED
Closed: 20 years ago
Flags: blocking-aviary1.0?
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: