Closed
Bug 1394745
Opened 7 years ago
Closed 7 years ago
Evicting the same origin string but in the different persistence type hits the assertion in MaybeUpdateSize()
Categories
(Core :: Storage: Quota Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: icrisan, Assigned: tt)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
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
Assignee | ||
Comment 1•7 years ago
|
||
The failure happens in QuotaManager.
No longer blocks: 1382598
Component: DOM: Service Workers → DOM: Quota Manager
Comment 3•7 years ago
|
||
Yeah, we shouldn't delete itself while freeing disk space.
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8902652 -
Flags: review?(jvarga)
Assignee | ||
Comment 9•7 years ago
|
||
try result for all platform with patches here and in bug 1382598: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39ee585cc5f8ef2a4fa26ec14513f3ae805bbcb
Updated•7 years ago
|
Attachment #8902652 -
Flags: review?(jvarga) → review+
Assignee | ||
Updated•7 years ago
|
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()
Assignee | ||
Comment 10•7 years ago
|
||
Update the commit message.
Attachment #8902652 -
Attachment is obsolete: true
Attachment #8903004 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
Mark as checkin-needed since the try looks ok
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
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•7 years 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
Comment 13•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•