612246 - /tmp/mozilla-media-cache directory not multi-user safe

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Stefan]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20100101 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20100101 Firefox/4.0b8pre

Audio and video do no longer work in current mozilla-central

Reproducible: Always

Steps to Reproduce:
1. open URL
2.
3.
Actual Results:  
Video is not played.

Expected Results:  
Video is played.

The first bad revision is:
changeset:   57450:65a5f743eaab
user:        Patrick Walton <[e-mail]>
date:        Sat Nov 13 09:27:52 2010 -0400
summary:     Bug 597136 - Use the new nsIScriptError2 interface instead of window.onerror to determine the Web Console to which messages should be routed. r=gavin a=blocking2.0:beta8+.

Comment 1

[Matthias Versen [:Matti]]

>Video is not played.
The given URL is audio only

This is wfm with Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101115 Firefox/4.0b8pre SeaMonkey/2.1b2pre

Comment 2

[Stefan]

Downloaded the nightly builds:

Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b8pre) Gecko/20101117 Firefox/4.0b8pre
Built from http://hg.mozilla.org/mozilla-central/rev/ad227939db82

Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101117 Firefox/4.0b8pre
Built from http://hg.mozilla.org/mozilla-central/rev/ad227939db82

Both do not play the OGG file.

Comment 3

[Stefan]

On 32-Bit 

Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101117 Firefox/4.0b8pre

does play the OGG file. Strange.

Comment 4

[Stefan]

I did a new bisection based on these 64-bit versions:

2010-11-07-03-mozilla-central last good 8cbe83542596
010-11-08-04-mozilla-central first bad a0826dcd6228

   The first bad revision is:
   changeset:   57091:a20460fb08dd
   user:        Chris Pearce <[e-mail]>
   date:        Sun Oct 17 08:58:20 2010 +1300
   summary:     Bug 572579 - Remove stray media cache files after 30s
   idle time on Windows. r=bsmedberg a=blocking2.0

Comment 5

[Stefan]

strace -fFeopen reveals whats going on here:

open("/tmp/mozilla-media-cache/media_cache", O_WRONLY|O_CREAT|O_EXCL|O_TRUNC, 0700) = -1 EACCES (Permission denied)

The reason for this failure is obvious: I have another FF instance running under a different user/group which created /tmp/mozilla-media-cache with permissions 0700. This precludes any new FF instance from opening the cache file.

Comment 6

[Stefan]

Proposal: append the user name to the directory

Comment 7

[Adam]

Attachment: Uses nsIUserInfo to get the username and appends to the cache directory name.

OS2 doesn't implement nsIUserInfo, and so on that platform the cache will continue to reside in ${TEMPDIR}/mozilla-media-cache; otherwise, it is ${TEMPDIR}/mozilla-media-cache-{username}.

Comment 8

[Stefan]

Great! Works (on Linux haven't tried with different OS).

BTW: Is it possible to delete the two directories

/tmp/mozilla-media-cache-$USERNAME/
/tmp/orbit-$USERNAME/

at least when the program ends?

Comment 9

[J M Cerqueira Esteves]

Got bitten by this one today. I installed the Firefox 4 binary for Linux i686 on a Debian Lenny system (under /usr/local), ran it successfully (including viewing HTML5 videos) from a test account, then failed to view HTML5 videos when running Firefox 4 from my regular account. Ended up finding that this was caused by a /tmp/mozilla-media-cache directory remaining from the first runs by the test user, thus still owned by it.

A way to mitigate this for now: running Firefox with TMPDIR defined as some directory below $HOME does force mozilla-media-cache to be created under it, but the same also happens with, at least, orbit-$USER -- any inconvenient from this?

Comment 10

[Justin Dolske [:Dolske]]

Actually, why is this stuff in /tmp at all? Shouldn't it be in the local profile (like the normal disk cache)?

Comment 11

[cajbir (:cajbir)]

Bug 572579 commment 28 suggests it was because of Fennec compatibility.

Comment 12

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

I see, we createUnique the media cache file, but not the directory, so if Firefox is run with two different users we fail :-(.

Per https://bugzilla.mozilla.org/show_bug.cgi?id=572579#c29 I think the thing to do here is
> I'd *prefer* to communicate the profile-local
> directory to the content process and have it always use that, but that's
> probably a bit complicated.

I'm not sure of the best place to put such an API. Benjamin, any suggestions? Maybe during content process startup we can pass down the name of a directory where the content process can put temporary files?

Comment 13

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

(I think that could be made to work with sandboxing quite easily too.)

Comment 16

[[:Aleksej]]

Comment on attachment 513617 [details] [diff] [review]
Uses nsIUserInfo to get the username and appends to the cache directory name.

Requesting review of the patch submitted over a year ago.

Can’t comment on the implementation, but the solution, even if there can be a better one, is good enough. It won’t add any additional directories in /tmp for people who are not affected by this bug, and additional directories are much less of an issue than this bug: imagine bumping into it unknowingly while testing video support in Firefox or video hosting software.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 513617 [details] [diff] [review]
Uses nsIUserInfo to get the username and appends to the cache directory name.

Review of attachment 513617 [details] [diff] [review]:
-----------------------------------------------------------------

I'm concerned about the username containing weird (non English/Latin) characters that wouldn't map to an ASCII string usable in a filename.

What we're really doing is skirting around the real issue; media cache files should be in the user-specific profile dir, not the system temp dir. But we can only do this in single process Gecko, not in e10s, but since e10s is on hold now for desktop, that's ok.

So...

::: content/media/nsMediaCache.cpp
@@ -556,3 @@
>  
>    nsCOMPtr<nsIFile> tmp;
>    nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(tmp));

Instead do:

#include "nsXULAppAPI.h"
// [...]

  // In single process Gecko, store the media cache in the profile
  // directory so that multiple users can use separate media caches
  // concurrently. However in multi-process Gecko there is no profile
  // dir, so just store it in the system temp dir instead.
  nsresult rv;
  nsCOMPtr<nsIFile> tmp;
  const char* dir = (XRE_GetProcessType() == GeckoProcessType_Content)
                  ? NS_OS_TEMP_DIR : NS_APP_USER_PROFILE_LOCAL_50_DIR;
  rv = NS_GetSpecialDirectory(dir, getter_AddRefs(tmp));
  NS_ENSURE_SUCCESS(rv,rv);

  nsCOMPtr<nsILocalFile> tmpFile = do_QueryInterface(tmp);
  NS_ENSURE_TRUE(tmpFile != nsnull, NS_ERROR_FAILURE);

::: xpcom/io/nsMediaCacheRemover.cpp
@@ +108,5 @@
>      if (idleSvc)
>        idleSvc->RemoveIdleObserver(this, TEMP_FILE_IDLE_TIME);
>  
>      nsCOMPtr<nsIFile> tmpDir;
>      nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR,

s/NS_OS_TEMP_DIR/NS_APP_USER_PROFILE_LOCAL_50_DIR/

Note nsMediaCacheRemover is Windows only, so you should add:
NS_ABORT_IF_FALSE(XRE_GetProcessType() == GeckoProcessType_Default,
                  "Need to update media cache file location"); 

That way if we ever turn on e10s on Windows, we'll know to come back and make the media cache work again.

Comment 18

[Adam]

Attachment: Updated based on review.

I've removed the username-based naming, and implemented your suggestion to use the profile directory for now, which will suffice until e10s.

It looks like modern Windows systems do support utf-8 in usernames (though I couldn't find a definitive specification).  POSIX compliance, on the other hand, specifies[1] that usernames conform to the Portable Filename Character Set[2].  So, if we do revisit that approach, we will be okay on POSIX-compliant platforms.

1. http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_429
2. http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_276

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 603038 [details] [diff] [review]
Updated based on review.

Review of attachment 603038 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50527c658f60

I changed your commit message to be more descriptive. Generally, checkin messages should summarize what the patch is doing, not reiterating the bug title.

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/50527c658f60

Comment 22

[Daniel Veditz [:dveditz]]

Please include a rationale for ESR inclusion with your nomination.

Comment 23

[Jory A. Pratt [:anarchy]]

As many linux distros are shipping esr releases for long term support would be great to have this landed on esr so each distro does not have to patch it in.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 603038 [details] [diff] [review]
Updated based on review.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 23.
User impact if declined: No HTML5 video for some users on multi-seat systems.
Fix Landed on Version: 13
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 25

[Alex Keybl [:akeybl]]

(In reply to Mike Hommey [:glandium] from comment #24)
> Risk to taking this patch (and alternatives if risky): None

Are you in a position to evaluate the risk of this patch? The fact that it's shipped does not necessarily mean it poses no risk to an earlier branch. If this has any possibility of regression, it definitely falls outside the scope of the ESR.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Alex Keybl [:akeybl] from comment #25)
> (In reply to Mike Hommey [:glandium] from comment #24)
> > Risk to taking this patch (and alternatives if risky): None
> 
> Are you in a position to evaluate the risk of this patch? The fact that it's
> shipped does not necessarily mean it poses no risk to an earlier branch. If
> this has any possibility of regression, it definitely falls outside the
> scope of the ESR.

The patch changes the location of the media cache to NS_APP_USER_PROFILE_LOCAL_50_DIR instead of NS_OS_TEMP_DIR for the main process (it doesn't change it for content processes, but while it mattered for e10s, it doesn't matter for oopp).

That location is used and filled in content/media/nsMediaCache.cpp, and its content is flushed on shutdown (there is a last-pb-context-exited observer). In practice, this means there is nothing interesting there after Firefox stops, and starting a new Firefox with the patch applied will just use a new location for its own media caching needs.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment 23 isn't enough to justify landing this for ESR without a clear sense of any regression risk. It will be fixed in the next major branching of ESR (with Firefox 17) in a few more release cycles. I think that linux distros that use the ESR branch can wait for that.