The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla13

Status

()

Core
Audio/Video
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Stefan, Assigned: hobophobe)

Tracking

({regression})

unspecified
mozilla13
x86_64
All
regression
Points:
---

Firefox Tracking Flags

(firefox-esr10- wontfix)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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 <pwalton@mozilla.com>
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+.
>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
Keywords: regression
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
On 32-Bit 

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

does play the OGG file. Strange.
(Reporter)

Comment 4

6 years ago
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 <chris@pearce.org.nz>
   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
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
Proposal: append the user name to the directory
Summary: audio/video do not work → /tmp/mozilla-media-cache directory not multi-user safe
(Assignee)

Comment 7

6 years ago
Created attachment 513617 [details] [diff] [review]
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}.
(Reporter)

Comment 8

6 years ago
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?
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?
Actually, why is this stuff in /tmp at all? Shouldn't it be in the local profile (like the normal disk cache)?
Component: General → Video/Audio
Product: Firefox → Core
QA Contact: general → video.audio

Comment 11

6 years ago
Bug 572579 commment 28 suggests it was because of Fennec compatibility.
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?
(I think that could be made to work with sandboxing quite easily too.)
Blocks: 572579
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Duplicate of this bug: 684368

Updated

5 years ago
Severity: normal → major
OS: Linux → All
Duplicate of this bug: 730656

Comment 16

5 years ago
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.
Attachment #513617 - Flags: review?(cpearce)
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.
Attachment #513617 - Flags: review?(cpearce) → review-
(Assignee)

Comment 18

5 years ago
Created attachment 603038 [details] [diff] [review]
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
Attachment #513617 - Attachment is obsolete: true
Attachment #603038 - Flags: review?(cpearce)
Comment on attachment 603038 [details] [diff] [review]
Updated based on review.

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

Great, thanks!
Attachment #603038 - Flags: review?(cpearce) → review+
Keywords: checkin-needed

Updated

5 years ago
Assignee: nobody → unusualtears
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50527c658f60
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
tracking-firefox-esr10: --- → ?
Please include a rationale for ESR inclusion with your nomination.
tracking-firefox-esr10: ? → ---

Comment 23

5 years ago
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.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?
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.
Attachment #603038 - Flags: approval-mozilla-esr10?
(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.
(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 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.
status-firefox-esr10: affected → wontfix
tracking-firefox-esr10: ? → -
Attachment #603038 - Flags: approval-mozilla-esr10?
You need to log in before you can comment on or make changes to this bug.