Bug 92166 – [Chromium] [DumpRenderTree] IndexedDB: Clear backing store after each test

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Joshua Bell]

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=89609 the introduction of an on-disk backing store for the Chromium DRT IndexedDB tests means that backing store state is preserved across tests. That can lead to bugs e.g.:

test1:
req = indexedDB.open('db', 1);
req.onupgradeneeded = function() { db = req.result; db.createObjectStore('foo'); }
req.onsuccess = function() { shouldBeNonNull("db"); }

test2:
req = indexedDB.open('db', 1);
req.onupgradeneeded = function() { db = req.result; db.createObjectStore('foo'); }
req.onsuccess = function() { shouldBeNonNull("db"); }

Since the database name and version is the same in the second test the "upgradeneeded" event will not fire, and hence "db" will never be assigned and the assertion will fail.

[Joshua Bell]

Created attachment 154182 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

I came up with a handful of approaches for doing this - I'm not sure which is best, and I'm not sure about the best way to implement most of them because the route between the TestShell and things like GroupSettings is mysterious. Guidance appreciated.

As background, the current temp dir used in DRT is allocated via TestWebKitPlatformSupport::idbFactory() - but that WebIDBFactory instance is reused for the lifetime of the DRT instance, which is problematic.

Proposal #1: 

This is the included patch - it set a unique temp directory for each test via new static API on WebIDBFactory. The change is is modeled after http://trac.webkit.org/changeset/88358 (the migration tests for sqlite->leveldb), so it's at least got some precedent. However, it does this extra work for every layout test(!); and exposes more public API methods. It works, but I'm not thrilled.

Proposal #2:

IDBFactory::open() already digs out a path via document->page()->group().groupSettings()->indexedDBDatabasePath() (there's similar plumbing for workers). Setting a new path for each test could be done via groupSettings()->setIndexedDBDatabasePath(). This has the advantage over Proposal #1 that it doesn't introduce new APIs, although it still requires per-test recycling of the temp directory. I'm unsure how to get from the TestShell to the page to make this chain of calls, though.

Proposal #3:

The fundamental reason the WebIDBFactory instance is retained across tests is because the same PageGroup is retained across tests. That smells like a bug to me, but I have no idea how to approach that one.

Proposal #4: 

If retaining the PageGroup is intentional (see Proposal #2), reset the pagegroup's IDBFactoryBackendInterface; this could be done via a new method e.g. PageGroupIndexedDatabase::resetFactoryBackend() which releases m_factoryBackend. The next time there's a call through to DOMWindowIndexedDatabase::webkitIndexedDB a new call to TestWebKitPlatformSupport::idbFactory() will occur.

Thoughts?

[Tony Chang]

One way to avoid a new public WebKit API method would be to add a function to Source/WebCore/testing/Internals.idl.  This is only exposed to testing.  This would likely work for something like proposal #2.

It's a bit unfortunate that we'd have to create a directory between every layout test.  Maybe we could add a Internals or LayoutTestController method for resetting the database and call it at the start of each IDB test?  Alternately, maybe you could have IDBFactory::open mark a bit in webcore that says that it's been used and have the reset code check to see if that bit is set (and re set the bit to false).

[Ojan Vafai]

(In reply to comment #4)
> Alternately, maybe you could have IDBFactory::open mark a bit in webcore that says that it's been used and have the reset code check to see if that bit is set (and re set the bit to false).

This would be my preference. This way the code only needs to be written once (not added to every test), avoiding a distributed burden.

[Joshua Bell]

Created attachment 157291 [details]
Patch

[Joshua Bell]

(In reply to comment #6)
> Created an attachment (id=157291) [details]
> Patch

Latest version adds a "dirty" flag which can be tested. It's done in the chromium WebKit public API rather than in WebCore itself - reasonable?

The one bit of nastiness is around the static locals. They will be accessed by Workers that call open (etc), so can't be defended by an isMainThread() test.

Hrm, on second thought that isn't safe, as there's an initialization race. Bleah, r-. I think this really does need to be plumbed all the way through to the page group.

[Ojan Vafai]

(In reply to comment #5)
> (In reply to comment #4)
> > Alternately, maybe you could have IDBFactory::open mark a bit in webcore that says that it's been used and have the reset code check to see if that bit is set (and re set the bit to false).
> 
> This would be my preference. This way the code only needs to be written once (not added to every test), avoiding a distributed burden.

The disadvantage to this approach is that we need to make sure every port does this. Seems like we should have a WebCore hook in general that does all the WebCore-related reseting that we need to do per test. Then every port can call that method and we don't need to have all these one-off changes for each port.