Bug 128282 – IDB: storage/indexeddb/mozilla/clear.html fails

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

IDB: storage/indexeddb/mozilla/clear.html fails

Crashes due to an assert.

ASSERTION FAILED: m_state == Unused || m_state == Running
/Volumes/Data/git/OpenSource/Source/WebCore/Modules/indexeddb/IDBTransactionBackend.cpp(217) : void WebCore::IDBTransactionBackend::commit()
1   0x110f656c0 WTFCrash
2   0x112f6c080 WebCore::IDBTransactionBackend::commit()
3   0x1139a0bf9 WebCore::IDBDatabaseBackend::commit(long long)
4   0x113436454 WebCore::IDBTransaction::setActive(bool)
5   0x113856ba6 WebCore::IDBPendingTransactionMonitor::deactivateNewTransactions()
6   0x112f62d39 WebCore::JSMainThreadExecState::didLeaveScriptContext()
7   0x112cdfb8f WebCore::JSMainThreadExecState::~JSMainThreadExecState()
8   0x112cdfae5 WebCore::JSMainThreadExecState::~JSMainThreadExecState()
9   0x112cdfa3f WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
10  0x112e5b1e4 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*)
11  0x1126e43d1 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&)
12  0x1126e3d1e WebCore::EventTarget::fireEventListeners(WebCore::Event*)
13  0x112258b89 WebCore::IDBEventDispatcher::dispatch(WebCore::Event*, WTF::Vector<WTF::RefPtr<WebCore::EventTarget>, 0ul, WTF::CrashOnOverflow>&)
14  0x113b0769d WebCore::IDBRequest::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
15  0x112b06807 WebCore::IDBOpenDBRequest::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
16  0x112b0684c non-virtual thunk to WebCore::IDBOpenDBRequest::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
17  0x112538524 WebCore::DocumentEventQueue::dispatchEvent(WebCore::Event&)
18  0x112538404 WebCore::DocumentEventQueue::pendingEventTimerFired()
19  0x11253c202 WebCore::DocumentEventQueue::Timer::fired()
20  0x113c259dc WebCore::ThreadTimers::sharedTimerFiredInternal()
21  0x113c25699 WebCore::ThreadTimers::sharedTimerFired()
22  0x1139556c3 WebCore::timerFired(__CFRunLoopTimer*, void*)
23  0x7fff90da7724 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
24  0x7fff90da725f __CFRunLoopDoTimer
25  0x7fff90e1876a __CFRunLoopDoTimers
26  0x7fff90d62aa5 __CFRunLoopRun
27  0x7fff90d62275 CFRunLoopRunSpecific
28  0x7fff90fd1f0d RunCurrentEventLoopInMode
29  0x7fff90fd1cb7 ReceiveNextEventCommon
30  0x7fff90fd1abc _BlockUntilNextEventMatchingListInModeWithFilter
31  0x7fff91a3028e _DPSNextEvent

[Radar WebKit Bug Importer]

<rdar://problem/15996653>

[Brady Eidson]

The ASSERTs were simply wrong in our multi-process universe, so stopping the crashing was a matter of updating the asserts.

The test is outdated.  It tests for a success condition no longer allowed for by the spec.

In updating the test for the current spec I uncovered another correctness bug (WebKit current gives NULL whereas the spec calls for UNDEFINED)

Uploading the current patch in progress so I can grab from a different machine later.

[Brady Eidson]

Created attachment 223283 [details]
Current patch (still has a layout test bug, so not ready for review/landing)

[Radar WebKit Bug Importer]

<rdar://problem/15997155>

[Brady Eidson]

Created attachment 223313 [details]
Patch v1 - Enable the test, update it to current spec, and make it pass.

[David Kilzer (:ddkilzer)]

Comment on attachment 223313 [details]
Patch v1 - Enable the test, update it to current spec, and make it pass.

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

r=me, but cq- to consider the question about converting a RefPtr<IDBKey> to a bool, and whether to use "!!" syntax for pointer conversion to bool.

> Source/WebCore/ChangeLog:10
> +        Update the value deserializer to take into account whether or not there was an IDBKey

Nit: Can haz period?

> Source/WebCore/ChangeLog:16
> +        * Modules/indexeddb/IDBRequest.cpp:
> +        (WebCore::IDBRequest::onSuccess): Call the new form of deserializeIDBValueBuffer

Ditto.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:290
> -    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer());
> +    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer(), key);

Technically, shouldn't this be "key.get()" to match the similar call below?  And would "!!key.get()" be clearer that you're converting a pointer into a bool?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:421
> -    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), buffer);
> +    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), buffer, key.get());

Would it be clearer to use "!!key.get()" here instead to show you're converting a pointer into a bool?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:311
> +    // If the key doesn't exist, then the value must be undefined (as opposed to null)

Nit: Plz add period.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:313
> +        // We either shouldn't have a buffer or it should be of size 0

Nit: Add period.

> Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp:104
> +    const char* modeString;

Should probably initialize this to NULL:

const char* modeString = NULL;

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:70
> +    // It's okay to not have a SQLite transaction or not have started it yet because its okay for a WebProcess

Typo:  "its" => "it's"

[Brady Eidson]

(In reply to comment #6)
> (From update of attachment 223313 [details])
>
> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:290
> > -    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer());
> > +    Deprecated::ScriptValue value = deserializeIDBValueBuffer(requestState(), backend->valueBuffer(), key);
> 
> Technically, shouldn't this be "key.get()" to match the similar call below? 

I guess the reason I wrote them differently is because this one is a RefPtr whereas the key.get() below is a PassRefPtr.  That said, they both have unspecifiedbooltype operators declared, so I'm not sure why I treat them differently in this context...

> And would "!!key.get()" be clearer that you're converting a pointer into a bool?

We have no coding style guideline about this, and in general I'm not sure when !! would be preferred with ptr -> bool conversion, the same way I'm not sure when it might be preferred with int -> bool conversion.

Searching for !! use in WebCore shows that we use it...  but not nearly as much as one might suspect.

I don't feel strongly about it one way or the other, but am annoyed that we don't have a standard.

[Brady Eidson]

http://trac.webkit.org/changeset/163538