1411578 - Tab crashes when disk out of size - temporary file not removed

Status: VERIFIED FIXED

(Core :: Audio/Video: Recording, defect, P2)

Description

[Adrian Florinescu [:aflorinescu]]

[Preconditions:]
Easier to reproduce when the system partition has little disk space available.

[Steps:]
1.Launch FF
2. Go to: https://jsfiddle.net/pehrsons/7kgvL48e/
3. -Select/Check the Video box (square box) and Select Video Source - Screen (circle box) - in the right side of the page
3'.-Select/Check the Video box (square box) and Select Audio Source - Microphone (circle box) - in the right side of the page
4.Press Start button
5. Continue recording until you run out of space on the system disk. 
 

[Actual Result:]
When the disk free space is consumed, the tab in which you were recording crashes.
The temporary directory (Windows 10 case: C:\Users\adrian.florinescu\AppData\Local\Temp\mozilla-temp-files) will still contain the recording, (tab crashing makes it impossible to clean up?).

Comment 1

[Andreas Pehrson [:pehrsons]]

Did you check whether this happens in 55 or 57? (note that 56 turned off the file storage for the blobs, and 57 came with a new backend for them, see bug 1403706)

If it does the blocker should be adjusted. My hunch is that this is not a regression from bug 1296531 but a more general problem.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Are we sure these temp files are cleaned up when we DON'T crash?

What's desired behavior here? In some ways, I could see someone being relieved to find the recording on disk if we crash. Just trying to get a sense of whether fixing the crash here is sufficient, or whether we should look at a cleanup-after-crash strategy.

Comment 3

[Andreas Pehrson [:pehrsons]]

I'm also not sure how much of this is a MediaRecorder problem and how much is a general MutableBlobStorage problem. Perhaps there's a write error to handle in MediaRecorder but otherwise this seems like something to handle in MutableBlobStorage. Baku, would that be your area?

Comment 4

[Andrea Marchesini [:baku]]

Can you please share a crash ID? You can find it in about:crash. Thanks.

Comment 5

[Adrian Florinescu [:aflorinescu]]

(In reply to Andrea Marchesini [:baku] from comment #4)
> Can you please share a crash ID? You can find it in about:crash. Thanks.
AFAIK the tab crashes do not log crashes.

Comment 6

[Andrea Marchesini [:baku]]

> The temporary directory (Windows 10 case:
> C:\Users\adrian.florinescu\AppData\Local\Temp\mozilla-temp-files) will still
> contain the recording, (tab crashing makes it impossible to clean up?).

On windows we have a component (nsAnonTempFileRemover) that tries to delete temporary files in that directory any 3 minutes when FF is in idle state: https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsAnonymousTemporaryFile.cpp#153-164

But if there is an handler pointing a file, that file is not deletable on windows.

Can I ask you to check something?

1. Can you please reproduce this issue and see if you can manually delete those files? If this happens, firefox, parent process, is not keeping those files open. And this is what I hope it happens. Otherwise we have a bug in MutableBlobStorage.

2. If 1. works fine, can you wait > 3 minutes and see if those files are deleted?

I use linux. I cannot reproduce this issue. I'm currently trying to reproduce the crash.

Comment 7

[Adrian Florinescu [:aflorinescu]]

Sure :baku. We are currently running the test suite for media recorder, but I do plan to reproduce this issue as soon as possible and I shall pay additional details to comment 6 details. (we didn't try on linux and mac yet, that shall follow).

Comment 8

[Andrea Marchesini [:baku]]

Attachment: mutable.patch

MutableBlobStorage can have multiple runnable scheduled to write data on the temporary file. But when the first fails, it calls ErrorPropagated(). This method sends the error to the parent process and it nullifies the mActor pointer.
The next runnable will call ErrorPropagated() again, but mActor is null, and FF crashes.

Comment 9

[Andrea Marchesini [:baku]]

I can reproduce the crash, as you can see by the patch. I still would like to know if the files are correctly deleted by FF parent process. Thanks!

Comment 10

[Adrian Florinescu [:aflorinescu]]

(In reply to Andrea Marchesini [:baku] from comment #9)
> I can reproduce the crash, as you can see by the patch. I still would like
> to know if the files are correctly deleted by FF parent process. Thanks!

On Windows 10 at least, the file is not deleted whatsoever after the tab crash. I've fumbled like 30' until I got hold of the disk location, so I am certain that the file was not deleted after tab crash and FF close/restart/etc.

What I've noticed is that if I have a blob created with the above recording test page will get cleared after I close the tab. I will try to close the tab while I lock the file to see if the parent process close will trigger the clear, but off the top of my head, I would speculate that it will not get cleaned by parent process after the lock is removed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4886cbad5a45
MutableBlobStorage should check if the error has been already propagated, r=smaug

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4886cbad5a45

Comment 13

[Andreas Pehrson [:pehrsons]]

Adrian, could you retest on latest Nightly?

baku, should this be uplifted?

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8922322 [details] [diff] [review]
mutable.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1403706
[User impact if declined]: a crash can occur for a UAF
[Is this code covered by automated tests?]: none
[Has the fix been verified in Nightly?]:  yes
[Needs manual test from QE? If yes, steps to reproduce]:  easy to reproduce following the STR
[List of other uplifts needed for the feature/fix]: 1403706
[Is the change risky?]: low
[Why is the change risky/not risky?]: just a null pointer check.
[String changes made/needed]: none

Comment 15

[Adrian Florinescu [:aflorinescu]]

We've tested the latest Nightly 58.0a1 20171029220112 on Windows 10/MacOsx 10.12 with the following results (using STR from comment 0):

Windows 10:
-Space is consumed until it hits 0 free space. 
-The browser console states: "Could not write session state file  Object { operation: "write", path: "C:\Users\adrian.florinescu\AppData\…", winLastError: 112, stack: "", fileName: "(unknown module)", lineNumber: undefined }   SessionFile.jsm:396 "  
-The tab containing the fiddle doesn't crash anymore, pressing Stop button will state: "Recording Error"
-Closing the tab/browser will not clear the space, temporary file is not removed.

Mac Osx10.12:
-Space is consumed, but apparently Osx somehow sprouts space, but what is different on Mac is that the temporary file vanishes when it hits the max available space size. 
-same "Recording Error" when pressing Stop button.

I'm not entirely sure what the patch fixes in order to verify just that? The tab crash? Or should it have additional effects?

Comment 16

[Andrea Marchesini [:baku]]

Let's fix this issue on a separate bug: bug 1412822

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8922322 [details] [diff] [review]
mutable.patch

Low risk, Beta57+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

AFAICT, the regressing patch only ever landed on 58?

Comment 19

[Andrea Marchesini [:baku]]

Yes, only 58.

Comment 20

[Andreas Pehrson [:pehrsons]]

Would be great to have this verified now that the dependencies have landed.

Comment 21

[Adrian Florinescu [:aflorinescu]]

I've verified the issue, taking in account fixes from bug 1412822 and bug 1412894 and I'm not so sure about the expected functionality.

So, using STR from comment 0, when reaching the point of no more disk space available I get (win10 for now):

- the test page throws Recording failed;
- the temporary file is immediately deleted;

In theory, I guess that's the place where we wanted to get with this issue, but from real world perspective, I would assume that we would want somehow to preserve the recording in the out of space scenario, while in the same time making sure that we are cleaning space occupied after the error is thrown.

Andreas, thoughts?

Comment 22

[Andreas Pehrson [:pehrsons]]

Thanks! I think that's reasonable behavior.

This seems like a pretty narrow case.
If the application was streaming the recorded content to a server it would use a timeslice and continuously clean up blobs.
If a timeslice was used and it was not streaming, only the last blob would be lost.
If no timeslice was used and we ran out of disk space (where I think it'll be most common), the app doesn't have much room to do something useful (like download the recording) since the disk is full anyway. There's a chance the user has multiple disks I suppose. Let's file a bug for tracking but it'll be low priority.

Comment 23

[Adrian Florinescu [:aflorinescu]]

Based on comment 21 and comment 22, I think once we verify the Ubuntu behavior this issue can be marked as verified. NI Iulia to verify this as part of Media Recorder Refactor beta test run.

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I can confirm that the behavior stated in comment 21 is also reproducible on Ubuntu 16.04 x64 using 58.0b12 (20171218174357).
Based on this and on the previous comment, I will mark Fx 58 as verified fixed.