WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
92166
[Chromium] [DumpRenderTree] IndexedDB: Clear backing store after each test
https://bugs.webkit.org/show_bug.cgi?id=92166
Summary
[Chromium] [DumpRenderTree] IndexedDB: Clear backing store after each test
Joshua Bell
Reported
2012-07-24 15:14:35 PDT
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.
Attachments
Patch
(7.47 KB, patch)
2012-07-24 17:13 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(14.18 KB, patch)
2012-08-08 14:21 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-07-24 17:13:04 PDT
Created
attachment 154182
[details]
Patch
WebKit Review Bot
Comment 2
2012-07-24 17:15:14 PDT
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
Joshua Bell
Comment 3
2012-07-24 17:32:02 PDT
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
Comment 4
2012-07-25 12:40:35 PDT
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
Comment 5
2012-08-08 11:25:56 PDT
(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
Comment 6
2012-08-08 14:21:06 PDT
Created
attachment 157291
[details]
Patch
Joshua Bell
Comment 7
2012-08-08 14:25:14 PDT
(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
Comment 8
2012-08-08 14:31:13 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug