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

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect)

Description

[Ioana Crisan]

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

Comment 1

[Tom Tung [:tt, :ttung]]

The failure happens in QuotaManager.

Comment 2

[Tom Tung [:tt, :ttung]]

Sorry, I somehow change the dependency...

Comment 3

[Jan Varga [:janv]]

Yeah, we shouldn't delete itself while freeing disk space.

Comment 4

[Tom Tung [:tt, :ttung]]

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.

Comment 5

[Tom Tung [:tt, :ttung]]

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

Comment 6

[Tom Tung [:tt, :ttung]]

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

Comment 7

[Tom Tung [:tt, :ttung]]

(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

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: Modify assertion to allow QM to evict the origin in the different persistence type.

Comment 9

[Tom Tung [:tt, :ttung]]

try result for all platform with patches here and in bug 1382598: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39ee585cc5f8ef2a4fa26ec14513f3ae805bbcb

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: [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.

Comment 11

[Tom Tung [:tt, :ttung]]

Mark as checkin-needed since the try looks ok

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/c71b4f46c8e1

Comment 14

[Tom Tung [:tt, :ttung]]

tracking this in storage meta bug