Closed Bug 240068 Opened 20 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)

Product:
Firefox
Component:
File Handling
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: spiritraveller, Assigned: bugs)

References

(
URL
)

Details

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

Attachments

(2 files)

The Invalid Metafile error when launching Realplayer 10 beta from Linux Firefox
83.56 KB, image/jpeg
Details
Fixes bug 240068 by making mFinalFileDestination /tmp/untitled
444 bytes, patch
Biesinger
: review-
bzbarsky
: superreview+
Details | Diff | Splinter Review 
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: