Closed Bug 1398231 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::cache::LockedDirectoryPaddingGet

Categories

(Core :: DOM: Core & HTML, defect)

57 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: tt)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-a4ab879c-30ea-4402-a626-725600170908.
=============================================================

There are 3 crashes from the same installation in nightly 57 with buildid 20170908100218. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1290481.

[1] https://hg.mozilla.org/mozilla-central/rev?node=de5a573de6632f96368f646ff3eba7ac9e2707cc
Flags: needinfo?(ttung)
This is asserting on:

  MOZ_DIAGNOSTIC_ASSERT(!DirectoryPaddingFileExists(aBaseDir, DirPaddingFile::TMP_FILE));

Given users and other applications might manipulate the file system we may not be able to enforce this.  We might need runtime checking to handle unexpected conditions like this.

Given this is Windows its possible we are failing to remove the temp file because of virus scanner holding it open or something.
I'll investigate this. 

(In reply to Ben Kelly [:bkelly] from comment #1)
Thanks for the hint!

> This is asserting on:
> 
>   MOZ_DIAGNOSTIC_ASSERT(!DirectoryPaddingFileExists(aBaseDir,
> DirPaddingFile::TMP_FILE));
> 
> Given users and other applications might manipulate the file system we may
> not be able to enforce this.  We might need runtime checking to handle
> unexpected conditions like this.
> 
> Given this is Windows its possible we are failing to remove the temp file
> because of virus scanner holding it open or something.

I did pass the aTemporaryFileExist to LockedDirectoryPaddingGet() [1] before calling LockedDirectoryPaddingGet() (where the assertion is hit). So, I'll try to check aTemporaryFileExist before this function and consider whether should I remove the assertion[2] in LockedDirectoryPaddingGet().

[1] https://hg.mozilla.org/mozilla-central/annotate/50857982881a/dom/cache/FileUtils.cpp#l781
[2] https://hg.mozilla.org/mozilla-central/annotate/50857982881a/dom/cache/FileUtils.cpp#l730
Assignee: nobody → ttung
Status: NEW → ASSIGNED
Flags: needinfo?(ttung)
It's not easy to write a test to verify this issue, but I'll keep finding if it's possible to do that. Maybe using File API to create temporary padding file between cache actions (e.g. cache.put).
(In reply to Tom Tung [:tt] (OoO: 9/22 ~ 9/29) from comment #2)
> I'll investigate this. 

Currently, there are four places[1] using LockedDirectoryPaddingGet() (A, B, C, D). 

A: [2] is the palce that makes this crash, so I try to check whether the temporary padding file exists before calling LockedDirectoryPaddingGet() which has an assertion for the temporary padding file should not be existed. And, the r? patches is mainly to fix this situation.

B: [3] is in the if-condition and it should be called when the temporary padding file doesn't exist.

C: [4] is protected by LockedDirectoryPaddingRestore() where deleting the temporary padding file. If the delete fail, [4] should 
be called.

D: [5] is called after deleting the temporary padding file, so I believe it's fine.
  
[1] http://searchfox.org/mozilla-central/search?q=LockedDirectoryPaddingGet&case=false&regexp=false&path= 
[2] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#789
[3] http://searchfox.org/mozilla-central/source/dom/cache/QuotaClient.cpp#144
[4] http://searchfox.org/mozilla-central/source/dom/cache/QuotaClient.cpp#157
[5] http://searchfox.org/mozilla-central/source/dom/cache/QuotaClient.cpp#392
Comment on attachment 8906205 [details]
Bug 1398231 - P1: Make file exist check before assertion.

https://reviewboard.mozilla.org/r/177972/#review183284
Attachment #8906205 - Flags: review?(bkelly) → review+
Comment on attachment 8906241 [details]
Bug 1398231 - P2: Add a test to verify cache work when there is a temporary padding file.

https://reviewboard.mozilla.org/r/177988/#review183286
Attachment #8906241 - Flags: review?(bkelly) → review+
Thanks for the reviews!
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e999670eb295
P1: Make file exist check before assertion. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/89abc58564c8
P2: Add a test to verify cache work when there is a temporary padding file. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/e999670eb295
https://hg.mozilla.org/mozilla-central/rev/89abc58564c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.