Closed Bug 1349746 Opened 7 years ago Closed 7 years ago

FileBlockCache can leak anonymous temporary file

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1349746 - Ensure we close media cache's temporary file if the cache shuts down while we're waiting for the FD to arrive.
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
While trying to debug an unrelated problem, I added a fatal assert in nsThread::PutEvent() to trigger when we dispatch a runnable to a thread which cannot run that event. This triggered in dom/media/test/test_load.html in FileBlockCache::SetCacheFile() when we're dispatching a promise resolve runnable:

https://treeherder.mozilla.org/logviewer.html#?job_id=85517776&repo=try&lineNumber=11852

This is because we must have shutdown the media cache after requesting an anonymous temporary file, but before the callback with the temporary file has come in.

So FileBlockCache::SetCacheFile() needs to handle the cache being closed before the callback runs.

This is fallout from bug 1347031.
I also need to reject the promise, else I'll get assertion failures about FileBlockCache::mInitPromise still being pending in FileBlockCache's destructor, e.g.:
https://treeherder.mozilla.org/logviewer.html#?job_id=85769586&repo=try&lineNumber=16128
Comment on attachment 8850221 [details]
Bug 1349746 - Ensure we close media cache's temporary file if the cache shuts down while we're waiting for the FD to arrive.

https://reviewboard.mozilla.org/r/122878/#review125180
Attachment #8850221 - Flags: review?(jwwang) → review+
D'oh, that doesn't work either, as we're still resolving in FileBlockCache::SetCacheFile(), which asserts because FileBlockCache::Close() has already run and has shutdown the GMP thread:

https://treeherder.mozilla.org/logviewer.html#?job_id=85790480&repo=try&lineNumber=9533

If we don't resolve or reject the init promise, we hit a !FileBlockCache::mInitPromise::IsPending() assert in the FileBlockCache's destructor. But we can't resolve/reject in FileBlockCache::SetCacheFile() if FileBlockCache::Close() has been called, as there's no thread to run the promise Then runnable on.

So we need to reject the FileBlockCache::mInitPromise in FileBlockCache::Close() if we've not yet received a callback with the file descriptor. That way the reject runnable has time to run, and we'll have fulfiled the init promise so we won't assert in the FileBlockCache's destructor.
https://hg.mozilla.org/try/file/2455c41489c595f5f683ded271f648cf1e479cca/xpcom/threads/nsThread.cpp#l733
Ah! The assertion is added recently which doesn't reconcile with the semantics of AbstractThread::Dispatch() which allows dispatch to fail silently if DontAssertDispatchSuccess is specified.

http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/xpcom/threads/MozPromise.h#399
In fact, MozPromise expects the Then runnable could fail to dispatch and requires the consumer to disconnect the Then value explicitly.

Another solution might be replace AbstractThread::CreateXPCOMThreadWrapper() with a TaskQueue which conforms to the semantics of AbstractThread::Dispatch().

Either way, we need to review the implementation of AbstractThread::CreateXPCOMThreadWrapper() and make it reconcile with the semantics of AbstractThread.
Chris, should we nominate this for tracking for the release?  Leaking the file handle seems like a bad regression.
Hi Chris,
Are you still working on this bug? It turns out this bug needs to be fixed before I can do some refactoring in FileBlockCache and then fix bug 1354389 which is required by quantum-flow.
Flags: needinfo?(cpearce)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> https://hg.mozilla.org/try/file/2455c41489c595f5f683ded271f648cf1e479cca/
> xpcom/threads/nsThread.cpp#l733
> Ah! The assertion is added recently which doesn't reconcile with the
> semantics of AbstractThread::Dispatch() which allows dispatch to fail
> silently if DontAssertDispatchSuccess is specified.

That assertion is in my local branch, it's not landed on central. I added it to my local repo because I was concerned that tasks required to shutdown the GMP service weren't being run because the GMP thread had been shutdown. That's how I discovered this.

So the fact that this assertion doesn't reconcile with DontAssertDispatchSuccess isn't surprising! Note that in the case where my assertion fires, we'll leak the file handle, which is what I'm trying to fix here.

> http://searchfox.org/mozilla-central/rev/
> c48398abd9f0f074c69f2223260939e30e8f99a8/xpcom/threads/MozPromise.h#399
> In fact, MozPromise expects the Then runnable could fail to dispatch and
> requires the consumer to disconnect the Then value explicitly.

Keeping a request tracker around isn't great as we can't disconnect cleanly from FileBlockCache::Close() as it must be disconnected from the thread its supposed to run on; the worker thread.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> Hi Chris,
> Are you still working on this bug? It turns out this bug needs to be fixed
> before I can do some refactoring in FileBlockCache and then fix bug 1354389
> which is required by quantum-flow.

Yes, I have a new patch that removes the promise and uses raw threading primitives. Figuring out how to make promises work was doing my head in. It seemed whichever way I turned I hit another edge case with promises, so it was simpler to do it the old fashioned way.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29d1b1eb21f3b5921045cc8b36120883c0c68b06
Flags: needinfo?(cpearce)
Attachment #8850221 - Attachment is obsolete: true
Comment on attachment 8855692 [details]
Bug 1349746 - Ensure we close media cache's temporary file if the cache shuts down while we're waiting for the FD to arrive.

https://reviewboard.mozilla.org/r/127572/#review130288
Attachment #8855692 - Flags: review?(jwwang) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f16602232666
Ensure we close media cache's temporary file if the cache shuts down while we're waiting for the FD to arrive. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/f16602232666
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: