Evicting the same origin string but in the different persistence type hits the assertion in MaybeUpdateSize()

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM: Quota Manager
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Ioana Crisan, Assigned: tt)

Tracking

(Blocks: 2 bugs)

57 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
1. Run the "storage_test_coverage_2" patch from bug #1382598 through Try on all the environments
2. Check the results

Expected results:
No debug failures are triggered.

Actual results:
Debug failures like https://treeherder.mozilla.org/logviewer.html#?job_id=126449819&repo=try&lineNumber=9320 are triggered in the Gecko code.
Please see the entire job https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4ae50f904676c18363e90d028a88fbc7e2a6d7
(Reporter)

Updated

10 months ago
Blocks: 1382598
The failure happens in QuotaManager.
No longer blocks: 1382598
Component: DOM: Service Workers → DOM: Quota Manager
Sorry, I somehow change the dependency...
Blocks: 1382598

Comment 3

10 months ago
Yeah, we shouldn't delete itself while freeing disk space.
The failure happens in eviction on all platforms. 

I guess storage.estimate() creates the temporary type of originInfo object and indexedDB creates the default type of originInfo object. After a while, the temporary originInfo is collected to remove when the QM get inactive origins to be evicted. Thus, the test hits the assertion (for having the same origin string). 

I'll test to verify whether it's correct or not.
It seems that it only happens on try. I cannot reproduce it on my desktop (ubuntu 14.04).

try with a quick patch to verify my guess for this issue: 
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6ab450dcf8bbf4ad15bbb6bb5347c6a85b5c3ab
I was in opt build, but I find out I can reproduce it on debug build. 

So, the reason is that:
First, testing script uses storage estimate() to create temporary origin and it also uses indexedDB to create default origin. (both origins are the same string)
Then, the testing script keeps storing objects by indexedDB until triggering eviction.
Next, GetInactiveOriginInfos() collects inactive origins by checking if the origin holds the directory lock. (at this time storage estimate() have released its directory lock)
Finally, the assertion checks if the evicted origin (created by storage.estimate(), temporary) is the same origin string with the directory lock(holding by indexedDB, default) and this hit the assertion.

I believe there are two issues in this bug.
1. storage.estimate() shouldn't create an empty temporary originInfo, and I've filed bug 1373183 for it.
2. Not sure if this is an issue. Should we evict an origin if the complementary type (default <-> temporary) of origin holds the directory lock? If so, we should modify the assertion on MaybeUpdateSize() [1] to check the persistence type as well. If not, we should skip this kind of origin if the complementary type of origin holds a directory lock when collecting inactive origins.

[1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3075-3076
(In reply to Tom Tung [:tt] from comment #5)
> try with a quick patch to verify my guess for this issue: 

try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=609a61b9083ee1d27a2de87d0ceb9e7d4cbc4792&selectedJob=126964941
Created attachment 8902652 [details] [diff] [review]
Modify assertion to allow QM to evict the origin in the different persistence type.
Attachment #8902652 - Flags: review?(jvarga)

Updated

10 months ago
Attachment #8902652 - Flags: review?(jvarga) → review+
Summary: Debug failures in Gecko code when running patches through Try → Evicting the same origin string but in the different persistence type hits the assertion in MaybeUpdateSize()
Created attachment 8903004 [details] [diff] [review]
[Final] Bug 1394745: Modify the assertion to allow the QM to evict the origin in the different persistence type. r=janv

Update the commit message.
Attachment #8902652 - Attachment is obsolete: true
Attachment #8903004 - Flags: review+
Mark as checkin-needed since the try looks ok
Keywords: checkin-needed
Attachment #8903004 - Attachment description: Bug 1394745: Modify the assertion to allow the QM to evict the origin in the different persistence type. r=janv → [Final] Bug 1394745: Modify the assertion to allow the QM to evict the origin in the different persistence type. r=janv

Comment 12

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c71b4f46c8e1
Modify the assertion to allow the QM to evict the origin in the different persistence type. r=janv
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c71b4f46c8e1
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
tracking this in storage meta bug
Blocks: 1254428
You need to log in before you can comment on or make changes to this bug.