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

RESOLVED FIXED in Firefox 57

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
16 hours ago

People

(Reporter: calixte, Assigned: tt)

Tracking

(Blocks 1 bug, {crash, regression})

57 Branch
mozilla57
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(2 attachments)

Reporter

Description

2 years ago
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.
Assignee

Comment 2

2 years ago
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)
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
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).
Assignee

Comment 7

2 years ago
(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 8

2 years ago
mozreview-review
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 9

2 years ago
mozreview-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+
Assignee

Comment 10

2 years ago
Thanks for the reviews!

Comment 11

2 years ago
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: 2 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.