Closed Bug 1068596 Opened 10 years ago Closed 9 years ago

On Windows, media file is locked after the web page referencing it has been unloaded/closed (until GC); can't rename/move

Categories

(Core :: Audio/Video, defect)

35 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: duanyao.ustc, Assigned: jwwang)

References

Details

Attachments

(3 files)

Attached file media-lock.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140916133140

Steps to reproduce:

1. On Windows, load a web page which contains a <video> element referencing a local file, say "video1.mp4".

2. Navigate the tab to "about:blank".

3. Try to rename file video1.mp4.

You can unzip the attachment and try the html/video files.



Actual results:

Renaming fails, even after minitues, because file video1.mp4 is still locked by firefox.

Workarounds:
1. Go to "about:memory", and click GC & CC & Minimize memory usage, then  renaming can succeed.

2. Close the tab and wait for seconds, renaming can also succeed.



Expected results:

Firefox should unlock local media files referenced by a web page immediately after the page has been unloaded.

Or even better, never locks local media files. I see Chrome never locks local media files.
I think firefox could
1) do GC more frequently, so that file handles hold by an unloaded page can be closed quickly.
2) open local media files in FILE_SHARE_DELETE mode, so that doesn't prevent removing/renaming them.
  see http://msdn.microsoft.com/en-us/library/aa363858%28v=vs.85%29.aspx
If I start playing the video, and then navigate the tab to "about:blank", the sound of the video (the attached video has no sound, you need another one) stops immediately. This indicates that firefox does detected that the <video> is not needed anymore, so stops the playback -- why not just close the file handle at this point too?
The pages are cached for a while so that you can quickly navigate to next/previous pages.
(In reply to JW Wang [:jwwang] from comment #3)
> The pages are cached for a while so that you can quickly navigate to
> next/previous pages.
Thanks for pointing out this!

I oberseves that in FF If a video is playing, navigating to another page cause the playback to pause, and navigating back cause the playback to resume. Chrome doesn't resume playback and re-initializes the video.

I personally prefer Chrome's behavior, because automatic resuming can be superising and annoying in some situation. If automatic resuming could be dropped, I think firefox don't have to cache the meaid file's handle, caching some metadata in memory should be enough.

I'm developing a firefox add-on to edit html and allowing inserting videos. When a user requests to drop current document, the editor navigates the iframe containing it to a blank page, then deletes the html and its resources. However I encoutered the problem that video files can't be deleted on Windows!
Component: Editor → Video/Audio
OS: All → Windows 7
Hardware: All → x86_64
I investgated IO operations of firefox and chrome when they handling a local media file, by Process Moniter.

Firefox openned a media file with these options:

  Desired Access: Generic Read, Disposition: Open, Options: Synchronous IO Non-Alert, Non-Directory File, Attributes: n/a, ShareMode: Read, Write, AllocationSize: n/a, OpenResult: Opened

Chrome openned a media file with these options:

  Desired Access: Generic Read, Disposition: Open, Options: Non-Directory File, Attributes: n/a, ShareMode: Read, Write, Delete, AllocationSize: n/a, OpenResult: Opened

Note that chrome specified the share mode "Delete", but firefox didn't. This is why chrome allows renaming/deleting a openned media file, while firefox doesn't.

I think firefox can do the same thing as chrome does, and thus this issue is fixed.
The bug is still under investigation.
As a workaround, you can try to call |v.src = ""| to detach the file from the media element.
(In reply to JW Wang [:jwwang] from comment #8)
> The bug is still under investigation.
> As a workaround, you can try to call |v.src = ""| to detach the file from
> the media element.

Thanks. We actually already do this, however it only patially workarounds the problem.
I noticed that even detached and unreachable media elements can still keep media files open for a while, so we'll have to call |v.src = ""| on media elements just before them become unreachable. However in our editor media elements may be removed at several sites, tracking all of them becomes dirty and frigle.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: On Windows, media file is locked for a long time after the web page referencing it has been unloaded → On Windows, media file is locked after the web page referencing it has been unloaded/closed (until GC); can't rename/move
This one is very sad bug. I just wanted to share my use case (Win7, x86_64):
When I have 50+ tabs opened, I have to search through tabs to find which one of them contains the local file (in order to delete/rename that file).
I'd like to investigate this bug in the source code, however I can't find the responisble code that open video files. Can anyone help? Thanks!
I think I have figured out the related code.

It seems this issue can be fixed by passing `aShareModeDelete = true` to `nsLocalFile::OpenNSPRFileDescMaybeShareDelete()` in `nsLocalFile::OpenNSPRFileDesc()`. After this change, a media file can be renamed/deleted even if it is playing in Firefox.

However, this change may affect most client codes of `nsLocalFile` on Windows, I'm not sure it would break things or not yet. Any idea?
Per comment 7.

The change is introduced in revision 14a2fe92d07b in bug 994190 where files are opened with FILE_SHARE_DELETE defaults to false.

This is inconsistent with nsLocalFile::OpenNSPRFileDesc on other platforms where FILE_SHARE_DELETE defaults to true.

Btw, Ben's (bug 994190's owner) bugzilla account is disabled. I wonder who we can ask about this change.
Hi Kyle,
Since you review bug 994190, do you have any idea about comment 14?
Flags: needinfo?(khuey)
(Leaving needinfo because I will talk to Ben irl.)

IIRC, the problem here is that on non-Windows platforms if someone deletes a file that you have an open fd you can still continue using that fd until you close it, whereas on Windows once the file is deleted your fd will stop working.  I'm curious what Chrome does if a web page has a reference to a File object and the underlying file is deleted.  Can you still read from or XHR the File?
Chrome can still continue playback even if you delete the file in the middle of playback on Linux. However, when you press F5 to reload the page, it won't play anymore. Since gizmo.mp4 is a small file, I guess Chrome caches the entire file to keep playback going even if the file is deleted.

I wonder how it would be for Windows Chrome for I don't have Windows at hand to give it a try.

Hi Duan,
Can you give it a try on Windows?
Flags: needinfo?(duanyao.ustc)
Hi,

With my patch above, Firefox behaves much like Chrome(tested on Windows 10): a media file continues playing even if renamed/deleted; after page reloading, an error is shown.

It seems on Windows, FILE_SHARE_DELETE makes the file handle behave like a *nix fd: it keeps valid even if the corresponding file is renamed/deleted.
Flags: needinfo?(duanyao.ustc)
Update: 

Chrome on Window actually shows an error a few seconds after renaming the media file that is playing:

  GET file:///D:/movie/xxx.mp4 net::ERR_FILE_NOT_FOUND

I guess Chrome may open/close a media file mutiple times during the playback.

MS Edge behaves like Firefox with my patch: renaming in the middle of playback have no visible effect.
(In reply to Duan Yao from comment #18)
> It seems on Windows, FILE_SHARE_DELETE makes the file handle behave like a
> *nix fd: it keeps valid even if the corresponding file is renamed/deleted.

Yes, this is correct.

Ultimately this is a decision that should be made in netwerk/protocol/file.  There's no reason media files should behave differently than the HTML document that loads them.  And as far as I can tell all the media code does here is call NS_NewChannel to load a file:// URI, so these decisions are all made in Necko in the file channel code.
Flags: needinfo?(khuey)
Actually, it does appear that media code is doing something different here because I can rename/delete the HTML document.  Is it holding the nsIChannel alive longer somehow?
https://hg.mozilla.org/mozilla-central/file/8a6045d14d6b/dom/media/MediaResource.cpp#l884

Yes. MediaResource has to keep it alive and read data from the channel from time to time.

As I said in comment 14, the behavior of nsLocalFile::OpenNSPRFileDesc on Windows is inconsistent with those on other platforms.

I think FILE_SHARE_DELETE should default to true and MediaResource will figure out it is a resource error if it can't read data from the channel.
(In reply to JW Wang [:jwwang] from comment #22)
> I think FILE_SHARE_DELETE should default to true and MediaResource will
> figure out it is a resource error if it can't read data from the channel.

That's a question for the Necko owners, IMO.  Certainly not for me :)
Sure. But it would help if we can contact Ben to understand the design decision of FILE_SHARE_DELETE so we can reason about it with the Necko owners.
[11:39:58] <_bent> you had some problem with FILE_DELETE_SHARED?'
[11:40:07] <_bent> ha
[11:40:09] <_bent> lucky duck
[11:40:37] <khuey> https://bugzilla.mozilla.org/show_bug.cgi?id=1068596#c16
[11:40:40] <khuey> and some preceding comments
[11:40:48] <khuey> I just don't remember why we did it in the first place
[11:40:56] <khuey> but what I wrote makes sense :P
[11:41:15] <_bent> oh, so on windows you can't delete a file if it's open
[11:41:19] <_bent> deleting just fails
[11:41:25] <_bent> the fd continues to be alive
[11:41:41] <_bent> the file_share_delete thing tries to make it more posix-y
[11:42:11] <_bent> so iirc the problem was blobs are held alive by JS
[11:42:45] <_bent> and we have some code that tries to delete files even if
                   blob refs to them exist
[11:43:09] <_bent> if the blob happens to have an open fd and be using it, like
                   reading on another thread,
[11:43:21] <_bent> then those delete calls fail only on windows
[11:43:49] <_bent> i think we decided that there wasn't a way to ensure that
                   deletes happen only after blob refs are gone
[11:44:02] <_bent> (outside of indexeddb that is)
[11:45:23] <khuey> ok
[11:45:40] <_bent> you should probably verify all of that though ;)
[11:45:42] <_bent> it's been a while
[11:46:09] <khuey> so comment 7 says we're *not* passing share mode delete
[11:46:37] <_bent> this is a media bug?
[11:46:49] <khuey> I have no idea
[11:46:50] <khuey> yes
[11:46:54] <_bent> hm
[11:47:57] <khuey> ok, so I bet media code is not using OpenBlahShareDelete
                   when it should be
[11:47:59] <khuey> or something
[11:48:09] <khuey> seems like everytime we open an fd from a blob we should use
                   that
[11:48:29] <_bent> but i thought blob code did that explicitly
[11:48:37] <khuey> it's in file streams
[11:48:38] <_bent> maybe media is opening itself somehow
[11:48:45] <khuey> yeah, I'm looking
[11:49:00] <_bent> in other words i thought we fixed this for all blobs already
[11:49:03] <khuey> oh, you know what
[11:49:07] <khuey> this doesn't involve blobs at all
[11:49:15] <khuey> it's just <video src="file://blah">
[11:49:19] <_bent> oh
[11:49:27] <_bent> yeah, pass to media folks then ;)
[11:49:39] <_bent> (or sounds like a simple fix)
[11:50:41] <khuey> yeah
[11:51:07] <_bent> the share_delete stuff on windows is really weird
[11:51:21] <khuey> indeed
By the way, Chrome open both HTML and media file with share mode read+write+delete; while MS Edge open HTML with share mode read, and media file with read+write+delete.

I think it is not harmful to open all local files with share mode read+write+delete by default?
Ok, I found the code:
https://hg.mozilla.org/mozilla-central/annotate/ba43a48d3c528cc956335793e02504e5ca2c149f/netwerk/base/nsFileStreams.cpp#l343

SHARE_DELETE is only enabled on Windows when mBehaviorFlags contains the flag. This is wrong and inconsistent across platforms.

On Windows: nsIFileInputStream::SHARE_DELETE not specified -> file opened without 
SHARE_DELETE.
On Linux: nsIFileInputStream::SHARE_DELETE not specified -> file opened with SHARE_DELETE (because this is the default).
We should have the Necko owner look at the problem in comment 27 which is Ben?
(In reply to Duan Yao from comment #26)
> By the way, Chrome open both HTML and media file with share mode
> read+write+delete; while MS Edge open HTML with share mode read, and media
> file with read+write+delete.
> 
> I think it is not harmful to open all local files with share mode
> read+write+delete by default?

It should be no harm. But I am more concerned with the inconsistency in comment 27.
(In reply to JW Wang [:jwwang] from comment #28)
> We should have the Necko owner look at the problem in comment 27 which is
> Ben?

Jason Duell.
Hi Jason,
Can you check the inconsistency in comment 27? Thanks.
Flags: needinfo?(jduell.mcbugs)
I'm not sure what to do here.  IIUC we could 1) teach the media code to close files earlier (i.e. not have to wait for a GC), or 2) have media open files with the SHARED_DELETE flag, or 3) change the default for all FileStreams to be SHARED_DELETE.

#3 seems like an easy global fix, but I'm unsure of possible fallout, since it doesn't appear to have the exact same semantics as a Unix open-and-delete, i.e. it appears from comment 16 that the file actually goes away when deleted so further reads/writes fail, which is not the Unix behavior.  So there's actually no way to get rid of some inconsistency between Unix and Windows here.

Roc: do you have any understanding of the best fix here?
Flags: needinfo?(jduell.mcbugs) → needinfo?(roc)
bsmedberg also knows a lot about windows file systems and their intricacies so he may have thoughts here.
Flags: needinfo?(benjamin)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #32)
> I'm not sure what to do here.  IIUC we could 1) teach the media code to
> close files earlier (i.e. not have to wait for a GC), or 2) have media open
> files with the SHARED_DELETE flag, or 3) change the default for all
> FileStreams to be SHARED_DELETE.
> 
> #3 seems like an easy global fix, but I'm unsure of possible fallout, since
> it doesn't appear to have the exact same semantics as a Unix
> open-and-delete, i.e. it appears from comment 16 that the file actually goes
> away when deleted so further reads/writes fail, which is not the Unix
> behavior.  So there's actually no way to get rid of some inconsistency
> between Unix and Windows here.
> 
> Roc: do you have any understanding of the best fix here?

AFAICT from comments in this bug and on the Web, Kyle was wrong in comment #16 and FILE_SHARE_DELETE does work like Unix fds: the file handle remains valid if the file is deleted.

I suggest we take option #3.
Flags: needinfo?(roc)
I think we should fix the inconsistency in comment 27 first which is not about media so the client of nsFileInputStream can get a consistent and expected behavior across all platforms.

For now, SHARE_DELETE is true on Linux even when nsIFileInputStream::SHARE_DELETE is not specified.

After that is fixed, we can then tell media code to pass nsIFileInputStream::SHARE_DELETE when opening a file to fix the problem of this bug.
Wait, I think we can just tell media code to pass nsIFileInputStream::SHARE_DELETE without waiting for the fix for comment 27. Let me try...
Hi Duan,
Can you check if this patch fix the locking problem for you? I don't have Windows at hand to give it a try. Thanks.
Flags: needinfo?(duanyao.ustc)
Hi JW Wang,
Your patch does fix this issue. Now media file opened with share mode read+write+delete, while html file with read+write.
Flags: needinfo?(duanyao.ustc)
Comment on attachment 8653237 [details] [diff] [review]
1068596_enable_share_delete.patch

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

Tell MedisResource to pass SHARE_DELETE anyway so the file can always be deleted without waiting for GC.
Attachment #8653237 - Flags: review?(roc)
Thanks for the review!
The crash is about audio channel and doesn't seem to be related to my changes... Will check it out.
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/c0665c2bd0ce
Assignee: nobody → jwwang
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
See Also: → 1326691
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: