Bug 97149 - Fix IndexedDB unit tests
Summary: Fix IndexedDB unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-19 16:51 PDT by David Grogan
Modified: 2012-09-20 14:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.56 KB, patch)
2012-09-19 16:57 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2012-09-19 18:41 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (2.94 KB, patch)
2012-09-20 13:59 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-09-19 16:51:32 PDT
Fix IndexedDB unit tests
Comment 1 David Grogan 2012-09-19 16:57:54 PDT
Created attachment 164804 [details]
Patch
Comment 2 David Grogan 2012-09-19 16:58:18 PDT
Josh, could you take a look at this?
Comment 3 Joshua Bell 2012-09-19 17:09:29 PDT
Comment on attachment 164804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164804&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:450
> +    if (m_version == NoStringVersion && m_intVersion == IDBDatabaseMetadata::NoIntVersion && m_transactionCoordinator) {

I'm uncomfortable with this. This is basically using m_transactionCoordinator as an "if null, I'm in test mode, so behave differently" flag which doesn't make for a good unit test; the behavior change has nothing to do with the coordinator itself (e.g. skipping a call, etc)

It looks like this introduces a way for tests to bypass the open/upgradeneeded pattern to simplify the test. Could the test call openConnectionWithVersion(cb1, cb2, 0) and get the behavior that way?
Comment 4 David Grogan 2012-09-19 18:39:45 PDT
Comment on attachment 164804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164804&action=review

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:450
>> +    if (m_version == NoStringVersion && m_intVersion == IDBDatabaseMetadata::NoIntVersion && m_transactionCoordinator) {
> 
> I'm uncomfortable with this. This is basically using m_transactionCoordinator as an "if null, I'm in test mode, so behave differently" flag which doesn't make for a good unit test; the behavior change has nothing to do with the coordinator itself (e.g. skipping a call, etc)
> 
> It looks like this introduces a way for tests to bypass the open/upgradeneeded pattern to simplify the test. Could the test call openConnectionWithVersion(cb1, cb2, 0) and get the behavior that way?

So, the behavior change was at least slightly related to the coordinator - it was the null transaction coordinator that caused an invalid memory access if an int version transaction was run.  But you're right about openConnectionWithVersion, it's possible to get the desired behavior by changing just test code, which seems a lot safer.
Comment 5 David Grogan 2012-09-19 18:41:28 PDT
Created attachment 164818 [details]
Patch
Comment 6 Joshua Bell 2012-09-19 19:43:22 PDT
(In reply to comment #4)
> 
> So, the behavior change was at least slightly related to the coordinator - it was the null transaction coordinator that caused an invalid memory access if an int version transaction was run. 

Thanks - I didn't trace it all the way through.

> But you're right about openConnectionWithVersion, it's possible to get the desired behavior by changing just test code, which seems a lot safer.

Cool - I wasn't sure. New patch LGTM (although the -1 magic number might catch someone's attention...)
Comment 7 David Grogan 2012-09-20 13:59:13 PDT
Created attachment 164977 [details]
Patch
Comment 8 David Grogan 2012-09-20 14:06:59 PDT
Nate, could you review this?  jsbell@ gave it the LGTM in c6
Comment 9 David Grogan 2012-09-20 14:09:48 PDT
Nate, never mind, just noticed you're gardening.

Tony, could you review this?
Comment 10 WebKit Review Bot 2012-09-20 14:45:34 PDT
Comment on attachment 164977 [details]
Patch

Clearing flags on attachment: 164977

Committed r129170: <http://trac.webkit.org/changeset/129170>
Comment 11 WebKit Review Bot 2012-09-20 14:45:38 PDT
All reviewed patches have been landed.  Closing bug.