IDBObjectStore::get should run in a transaction.
Created attachment 65588 [details] Patch
Not ready for review yet.
Created attachment 66964 [details] Patch
Hi Jeremy, Andrei, As suggested, I took over this patch, hopefully we'll be able to either submit by EOW or at least leave in a good shape. :) My patch is essentially Andrei's, with the following changes: 1. Added tests. 2. Fixed / improved a couple of existing tests. 3. Added a didCompleteEventsForTransaction so that IDBRequest can notify the transactions that they can either step to the next set of tasks, or commit if there's nothing else pending.
(In reply to comment #4) > Hi Jeremy, Andrei, > > As suggested, I took over this patch, hopefully we'll be able to either submit by EOW or at least leave in a good shape. :) > > My patch is essentially Andrei's, with the following changes: > 1. Added tests. > 2. Fixed / improved a couple of existing tests. > 3. Added a didCompleteEventsForTransaction so that IDBRequest can notify the transactions that they can either step to the next set of tasks, or commit if there's nothing else pending. oh, forgot: 4. I did a minor refactoring / simplification on V8Proxy / IDBPendingTransactionMonitor
Attachment 66964 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3899315
Comment on attachment 66964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66964&action=prettypatch > LayoutTests/storage/indexeddb/objectstore-basics-expected.txt:-142 > - these lines should be here > LayoutTests/storage/indexeddb/objectstore-removeobjectstore.html:78 > + window.setTimeout('removeObjectStore()', 0); Note that this will force the transaction to commit, but it won't necessarily have completed after this. BUT you are guaranteed that it will before the removal happens. Maybe just update the comment? > LayoutTests/storage/indexeddb/objectstore-removeobjectstore.html:113 > + shouldBe("event.code", "5"); Fix this fixme. > LayoutTests/storage/indexeddb/script-tests/transaction-get.js:12 > + shouldBe('exc.code', '9'); use the constant > LayoutTests/storage/indexeddb/script-tests/transaction-get.js:34 > + transaction.onabort = unexpectedErrorCallback; Does the abort have an error code and message? If not, you should make a new unexpectedAbortCallback method. > LayoutTests/storage/indexeddb/transaction-get.html:11 > +<script src="script-tests/transaction-get.js"></script> I'm about to submit a change that gets rid of script tests and converts them over to normal html pages. Please convert your new one and merge your changes to the .js files to the associated html files? > WebCore/bindings/v8/V8Proxy.cpp:655 > + IDBPendingTransactionMonitor::abortPendingTransactions(page->group().idbFactory()); I'd rather this still call some blazingly fast method (completely inline) method to decide whether we need to call into abortPendingTransactions since this will be called _every_ single time we leave JavaScript. > WebCore/storage/IDBDatabase.cpp:60 > + // FIXME: remove this method. Maybe describe why and move it to just above the method? And capitalize R. > WebCore/storage/IDBDatabase.cpp:63 > + return IDBObjectStore::create(objectStore.release(), -1); -1 should be some constant somewhere > WebCore/storage/IDBDatabaseBackendImpl.cpp:153 > + // FIXME: Do not expose this method via IDL. This fixme seems unneeded. Maybe add a fixme to remove it from the interface if appropriate tho? > WebCore/storage/IDBDatabaseBackendImpl.cpp:157 > + objectStore->setTransaction(transaction); The backend object should not store the transaction since multiple transactions can be associated with one backend at once. Only the frontend objhects should store it. > WebCore/storage/IDBFactoryBackendImpl.cpp:150 > + m_transactionCoordinator->didCompleteEventsForTransaction(transactionID); Is there any reason why the coordinators should be shared between databases. > WebCore/storage/IDBObjectStore.cpp:128 > + if (!m_isTransactionPending) Pending is so generic. m_isTransactionScheduleForExecution? :-) m_isTransactionInPurgatory m_inPurgatory etc btw, this really should be a per-transaction value, not a per object store. Maybe we should pass around pointers rather than IDs. Even though that means more wrapping/unwrapping. > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:81 > + // FIXME: implement use complete sentences comments so far
Andrei, Marcus and I talked, and I think you should take over the patch at this point. This way it's at a known good stable point for you to take over (so you waste less time figuring out which way is up).
Created attachment 68067 [details] Patch
Attachment 68067 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/storage/IDBObjectStoreBackendImpl.cpp:95: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. WebCore/storage/IDBObjectStoreBackendImpl.h:78: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/storage/IDBObjectStoreBackendImpl.h:81: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/storage/IDBObjectStoreBackendImpl.h:93: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 68067 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4093009
Comment on attachment 68067 [details] Patch a few of my comments View in context: https://bugs.webkit.org/attachment.cgi?id=68067&action=review > WebCore/storage/IDBDatabase.cpp:60 > + // FIXME: remove this method. put above function definition > WebCore/storage/IDBDatabaseBackendImpl.cpp:153 > + // FIXME: Do not expose this method via IDL. move up > WebCore/storage/IDBDatabaseBackendImpl.cpp:154 > ASSERT_UNUSED(mode, !mode); // FIXME: Handle non-standard modes. Change the fixme to be a "delete this". It should be deleted when we remove it from the IDL. > WebCore/storage/IDBDatabaseBackendImpl.cpp:156 > + return objectStore.release(); For stuff like this, I usually just do a .get() on what's returned which will coerce itself type wise...of course that will cause a needless ref count/de-ref. Your choice. > WebCore/storage/IDBDatabaseBackendInterface.h:58 > + virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0) = 0; Put a fixme in to remove the = 0 since it's just a compat hack. > WebCore/storage/IDBFactoryBackendImpl.cpp:147 > + m_transactionCoordinator->abortPendingTransactions(pendingIDs); Why do we still use ids here? THis is in the backendImpl, so we shouldn't need to. > WebCore/storage/IDBObjectStore.cpp:70 > return request; .release() > WebCore/storage/IDBObjectStoreBackendImpl.cpp:84 > + if (transaction) { Would this ever not be true? > WebCore/storage/IDBObjectStoreBackendImpl.cpp:90 > + OwnPtr<ScriptExecutionContext::Task> getTask = adoptPtr(newTask(this, &IDBObjectStoreBackendImpl::get, key, callbacks)); Why do you have to adopt the newTask pointer? Shouldn't it return a passOwnPtr? > WebCore/storage/IDBObjectStoreBackendImpl.cpp:91 > + transaction->scheduleTask(getTask.leakPtr()); Why do you call leak? Can't you pass a passownptr? > WebCore/storage/IDBObjectStoreBackendImpl.cpp:354 > + delete > WebCore/storage/IDBObjectStoreBackendImpl.h:75 > + class IDBTask : public ScriptExecutionContext::Task { I think add a create method that returns a passOwnPtr > WebCore/storage/IDBObjectStoreBackendImpl.h:78 > + : m_obj(obj), m_method(method), m_param1(param1), m_param2(param2) { { on new line > WebCore/storage/IDBObjectStoreBackendImpl.h:81 > + virtual void performTask(ScriptExecutionContext*) { ditto > WebCore/storage/IDBObjectStoreBackendImpl.h:84 > + } newline after > WebCore/storage/IDBObjectStoreBackendImpl.h:93 > + ScriptExecutionContext::Task* newTask(T* object, Method method, const Param1& param1, const Param2& param2) { Should return a passOwnPtr > WebCore/storage/IDBObjectStoreBackendImpl.h:94 > + return new IDBTask<T, Method, Param1, Param2 >(object, method, param1, param2); no space after Param2 > WebCore/storage/IDBObjectStoreBackendImpl.h:106 > + no newline > WebCore/storage/IDBPendingTransactionMonitor.cpp:46 > + if (!m_ids) tabbed in too much > WebCore/storage/IDBPendingTransactionMonitor.cpp:64 > + if (m_ids) { Can't you just bail if this is true? If so, then you can declare the vector below > WebCore/storage/IDBPendingTransactionMonitor.cpp:69 > + if (pendingIds.size()) { Why would this be true and above wouldn't? > WebCore/storage/IDBPendingTransactionMonitor.cpp:75 > +void IDBPendingTransactionMonitor::didCompleteEventsForTransaction(int id, IDBFactoryBackendInterface* idbFactory) why are we using an id? > WebCore/storage/IDBRequest.cpp:60 > + if (transactionId) one place you do > 0 one place just check if it's == 0...which one is right? > WebCore/storage/IDBRequest.cpp:110 > + // FIXME: the transaction id should be the one of the setVersion transaction. True, but only because this is only used by a method that is always executed in a setVersion transaction. Maybe make the comment a bit more clear. > WebCore/storage/IDBRequest.cpp:172 > + if (m_transactionId > 0) { one place you do > 0 one place just check if it's == 0...which one is right? > WebCore/storage/IDBRequest.cpp:173 > + // Now that we processed all pending events, let the transaction monitor maybe wrap to 2 lines rather than 3? > WebCore/storage/IDBRequest.cpp:176 > + ASSERT(scriptExecutionContext()->isDocument()); Add a FIXME for handling the workers case. > WebCore/storage/IDBRequest.h:40 > +#include "IDBTransaction.h" not necessary > WebCore/storage/IDBTransaction.cpp:69 > + if (!objectStoreBackend.get()) { don't need the .get() > WebCore/storage/IDBTransaction.cpp:70 > + throwError(NOT_SUPPORTED_ERR); Do you need to add the string for this error somewhere? > WebCore/storage/IDBTransactionBackendImpl.cpp:51 > + , m_timer(this, &IDBTransactionBackendImpl::timerFired) The timer is just here until we add threading, right? Maybe add a FIXME? > WebCore/storage/IDBTransactionBackendImpl.cpp:65 > + newline doesn't really seem necessary (maybe delete?) > WebCore/storage/IDBTransactionBackendImpl.cpp:67 > + newline doesn't really seem necessary (maybe delete?) > WebCore/storage/IDBTransactionBackendImpl.cpp:72 > +void IDBTransactionBackendImpl::step() step doesn't seem like a very good name > WebCore/storage/IDBTransactionBackendImpl.cpp:75 > + newline doesn't really seem necessary (maybe delete?) > WebCore/storage/IDBTransactionBackendImpl.cpp:76 > + if (!m_taskQueue.isEmpty()) { if / else seems cleaner and less lines of code > WebCore/storage/IDBTransactionBackendImpl.cpp:101 > + m_timer.startOneShot(0); should we verify that it's not currently active? > WebCore/storage/IDBTransactionBackendInterface.h:54 > + virtual void step() = 0; step is not very descriptive > WebCore/storage/IDBTransactionCoordinator.cpp:61 > + RefPtr<IDBTransactionBackendInterface> transaction = m_pendingTransactions.get(id); It doesn't need to be removed? > WebCore/storage/IDBTransactionCoordinator.cpp:62 > + ASSERT(transaction); not necessary > WebCore/storage/IDBTransactionCoordinator.cpp:69 > + if (m_runningTransactions.contains(transactionID)) { When can this be false? Should we assert it's not one of the other types? > WebCore/storage/IDBTransactionCoordinator.cpp:70 > + RefPtr<IDBTransactionBackendInterface> transaction = m_runningTransactions.get(transactionID); 4 spaces > WebCore/storage/IDBTransactionCoordinator.cpp:92 > + // Remove from pending transactions list . at end > WebCore/storage/IDBTransactionCoordinator.cpp:123 > + // FIXME: this would allocate a thread to the next transaction Capital T on This. Maybe s/would/should/? > WebCore/storage/IDBTransactionCoordinator.cpp:126 > + if (m_startedTransactions.isEmpty()) maybe if (... || ...) return ? > WebCore/storage/IDBTransactionCoordinator.cpp:132 > + IDBTransactionBackendInterface* transactionPtr = *(m_startedTransactionQueue.begin()); I believe you don't need ()'s > WebCore/storage/IDBTransactionCoordinator.cpp:134 > + RefPtr<IDBTransactionBackendInterface> transaction = m_startedTransactions.get(transactionPtr->id()); transaction.get() should == transactionPtr, right? My suggestion: RefPtr<> transaction = *_startedTransactionQueue.begin(); ASSERT(transaction.get() == m_startedTransactions.get(transaction->id()).get()); ... > WebCore/storage/IDBTransactionCoordinator.cpp:136 > + // Add to running transactions list. comment isn't useful > WebCore/storage/IDBTransactionCoordinator.cpp:140 > + static_cast<IDBTransactionBackendImpl*>(transactionPtr)->run(); Don't do this. I guess you should be storing impl ptrs rather than interface ptrs? > WebCore/storage/IDBTransactionCoordinator.h:71 > + ListHashSet<IDBTransactionBackendInterface*> m_startedTransactionQueue; Maybe this should be a refptr too...just to be safe? If not, maybe we can put in more asserts to verify that anything in here is also in m_startedTransactions? > WebKit/chromium/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=44700 You need a lot more changelog here (but just about the WEbKit parts) > WebKit/chromium/public/WebIDBObjectStore.h:61 > WEBKIT_ASSERT_NOT_REACHED(); put on same line for void methods. > WebKit/chromium/src/IDBObjectStoreProxy.cpp:77 > + OwnPtr<WebKit::WebIDBTransactionImpl> transPtr = adoptPtr(new WebKit::WebIDBTransactionImpl(transaction)); Noooooo WebKit::WebIDBTransactionImpl webTransaction = static_cast<IDBTransactionBackendProxy*>(transaction)->getWebObject() (or something like that). > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:79 > +void IDBTransactionBackendProxy::step() hmmm.....this is probably why we should stick with ids between the frontend and backend interfaces. > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:84 > +bool IDBTransactionBackendProxy::isFinished() const lets try to get rid of these > WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:69 > +void WebIDBObjectStoreImpl::get(const WebIDBKey& key, WebIDBCallbacks* callbacks, const WebIDBTransaction& transaction) Ok...I think I like this afterall....basically the semantics we have is pass by reference when not transfering ownership and otherwise pass by ptr > WebKit/chromium/src/WebIDBTransactionImpl.cpp:82 > + extra new line > WebKit/chromium/src/WebIDBTransactionImpl.h:50 > +#if WEBKIT_IMPLEMENTATION this is always true in src
Comment on attachment 68067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68067&action=review > LayoutTests/storage/indexeddb/objectstore-basics-expected.txt:116 > +db.transaction() hu? does this work? Don't you need to save it to a variable? ideally use db.transaction() too? > LayoutTests/storage/indexeddb/objectstore-removeobjectstore.html:73 > +function indexAdded() Half the layout tests are named for what just happened and the other half are what should happen next. In this place, it's the latter, so name it something like commitTransaction > LayoutTests/storage/indexeddb/transaction-get.html:18 > + debug("Accessing a committed transaction should throw"); This should be a failure, not a debug It's ok to check in a failing result if that's what it should be. > LayoutTests/storage/indexeddb/transaction-get.html:22 > + shouldBe('exc.code', '9'); use the constant > LayoutTests/storage/indexeddb/transaction-get.html:27 > +function nonExistingKey() { all functions should put { on the next line > LayoutTests/storage/indexeddb/transaction-get.html:41 > + transaction.onabort = unexpectedErrorCallback; newline after this maybe? > LayoutTests/storage/indexeddb/transaction-get.html:44 > + var result = store.get('myKey'); put in evalAndLog > LayoutTests/storage/indexeddb/transaction-get.html:49 > + var emptyResult = store.get('nonExistingKey'); put at the end of gotValue (yes, this is supposed to work, but that'll make it easier to read) > LayoutTests/storage/indexeddb/transaction-get.html:80 > +function test() { From now on, please write new tests from top down in terms of execution and function names saying what just happened, please. > WebCore/storage/IDBTransactionBackendImpl.cpp:115 > +void IDBTransactionBackendImpl::runNextTasks() I don't like this function name. execute tasks or something? Or just move it into the timer function? > WebCore/storage/IDBTransactionBackendImpl.cpp:121 > + // pump the task queue dry Use complete sentences. Of course this comment doesn't seem necessary. > WebCore/storage/IDBTransactionBackendImpl.cpp:123 > + queue.swap(m_taskQueue); Is there a reason we need to do this? Even if stuff is appended, we can keep running them, right? > WebCore/storage/IDBTransactionBackendImpl.cpp:125 > + OwnPtr<ScriptExecutionContext::Task> task(queue.first().release()); personally I prefer = style initialization, but if you like this better it's fine > WebCore/storage/IDBTransactionBackendImpl.h:72 > + bool m_started; Please use enums to make this a proper state machine.
All fixed, except for the below: (In reply to comment #12) > > WebCore/storage/IDBObjectStore.cpp:70 > > return request; > > .release() > Why release? The object store is associated with this transaction and other operations may follow. > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:84 > > + if (transaction) { > > Would this ever not be true? > Yes, we use this to differentiate between the method being called by the object store or by the transaction. > > WebCore/storage/IDBTransaction.cpp:70 > > + throwError(NOT_SUPPORTED_ERR); > > Do you need to add the string for this error somewhere? > I don't think so, it's a standard exception.
Created attachment 68163 [details] Patch
(In reply to comment #13) > (From update of attachment 68067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68067&action=review > > > LayoutTests/storage/indexeddb/objectstore-basics-expected.txt:116 > > +db.transaction() > > hu? does this work? Don't you need to save it to a variable? ideally use db.transaction() too? > Yes, you're reading the expected output, not the actual test. > > > LayoutTests/storage/indexeddb/transaction-get.html:18 > > + debug("Accessing a committed transaction should throw"); > > This should be a failure, not a debug > > It's ok to check in a failing result if that's what it should be. > > > LayoutTests/storage/indexeddb/transaction-get.html:22 > > + shouldBe('exc.code', '9'); > > use the constant > > > LayoutTests/storage/indexeddb/transaction-get.html:27 > > +function nonExistingKey() { > > all functions should put { on the next line > > > LayoutTests/storage/indexeddb/transaction-get.html:41 > > + transaction.onabort = unexpectedErrorCallback; > > newline after this maybe? > > > LayoutTests/storage/indexeddb/transaction-get.html:44 > > + var result = store.get('myKey'); > > put in evalAndLog > > > LayoutTests/storage/indexeddb/transaction-get.html:49 > > + var emptyResult = store.get('nonExistingKey'); > > put at the end of gotValue (yes, this is supposed to work, but that'll make it easier to read) > > > LayoutTests/storage/indexeddb/transaction-get.html:80 > > +function test() { > > From now on, please write new tests from top down in terms of execution and function names saying what just happened, please. > These were added in the previous patch. Will look and fix tomorrow.
Attachment 68163 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3998098
(In reply to comment #14) > All fixed, except for the below: > > (In reply to comment #12) > > > WebCore/storage/IDBObjectStore.cpp:70 > > > return request; > > > > .release() > > > > Why release? The object store is associated with this transaction and other operations may follow. Release just releases one ref as a pass ref pointer. If your last use of a RefPtr is use variable in something that takes a passRefPtr, you should always call .release() so it doesn't have to ref it only to have the RefPtr get rid of its ref a moment later because it's gone out of scope. > > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:84 > > > + if (transaction) { > > > > Would this ever not be true? > > > > > Yes, we use this to differentiate between the method being called by the object store or by the transaction. Then it's a compat hack? If so, add a fixme. > > > WebCore/storage/IDBTransaction.cpp:70 > > > + throwError(NOT_SUPPORTED_ERR); > > > > Do you need to add the string for this error somewhere? > > > > I don't think so, it's a standard exception. Wait...why didn't you use an IndexedDB exception code? This one's probably not valid. (In reply to comment #16) > (In reply to comment #13) > > > > > LayoutTests/storage/indexeddb/transaction-get.html:18 > > > + debug("Accessing a committed transaction should throw"); > > > > This should be a failure, not a debug > > > > It's ok to check in a failing result if that's what it should be. > > > > > LayoutTests/storage/indexeddb/transaction-get.html:22 > > > + shouldBe('exc.code', '9'); > > > > use the constant > > > > > LayoutTests/storage/indexeddb/transaction-get.html:27 > > > +function nonExistingKey() { > > > > all functions should put { on the next line > > > > > LayoutTests/storage/indexeddb/transaction-get.html:41 > > > + transaction.onabort = unexpectedErrorCallback; > > > > newline after this maybe? > > > > > LayoutTests/storage/indexeddb/transaction-get.html:44 > > > + var result = store.get('myKey'); > > > > put in evalAndLog > > > > > LayoutTests/storage/indexeddb/transaction-get.html:49 > > > + var emptyResult = store.get('nonExistingKey'); > > > > put at the end of gotValue (yes, this is supposed to work, but that'll make it easier to read) > > > > > LayoutTests/storage/indexeddb/transaction-get.html:80 > > > +function test() { > > > > From now on, please write new tests from top down in terms of execution and function names saying what just happened, please. > > > > These were added in the previous patch. Will look and fix tomorrow. sounds good
Comment on attachment 68163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68163&action=review > WebCore/storage/IDBDatabaseBackendImpl.cpp:154 > + ASSERT_UNUSED(mode, !mode); // FIXME: Delete this. Make it more clear that we want to delete the param, not the line of code. > WebCore/storage/IDBDatabaseBackendInterface.h:58 > + // FIXME: remove the default value for the transaction parameter. Make it more clear when this will be possible. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:84 > + if (transaction) { FIXME: This is only necessary until .... > WebCore/storage/IDBPendingTransactionMonitor.cpp:40 > + m_transactions = new Vector<IDBTransactionBackendInterface*>(); NOOOOOO manual memory management! Why make the vector like this? at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to? > WebCore/storage/IDBPendingTransactionMonitor.cpp:56 > + delete m_transactions; NOOOOO > WebCore/storage/IDBPendingTransactionMonitor.cpp:68 > + delete m_transactions; NOOOOOO > WebCore/storage/IDBPendingTransactionMonitor.h:58 > + static Vector<IDBTransactionBackendInterface*>* m_transactions; monitor is in browser process, right? if so, it should have BackendImpl pointers....or maybe even refs? > WebCore/storage/IDBRequest.cpp:174 > + if (m_transaction.get()) { no .get() > WebCore/storage/IDBRequest.cpp:180 > + m_transaction.clear(); why clear it? i know we clear objects in a few places, but i'm wondering if it's really worth bothering > WebCore/storage/IDBRequest.h:97 > + RefPtr<IDBTransactionBackendInterface> m_transaction; store an id here...and anywhere else other than backend impls > WebCore/storage/IDBTransaction.cpp:70 > + throwError(NOT_SUPPORTED_ERR); as i mentioned in the other thread, i think this is the wrong error...we should use some idb error. unknown if there isn't one specced yet (and file a spec bug) > WebCore/storage/IDBTransactionBackendImpl.cpp:68 > + if (m_state == IDLE) some of your methods here can always be called (like abort) and bail if it's not in a state where that makes sense...and some like start assume you only call it when it makes sense. Maybe you should be consistent? > WebCore/storage/IDBTransactionBackendImpl.cpp:118 > + remove newline to be consistient with method below? > WebCore/storage/IDBTransactionBackendImpl.cpp:137 > + m_database.clear(); is there any reason we need to do the .clear() if not, it might be better to get rid of didFinishTransaction here and just inline the trnasaction cordinatior calls to match what you do in stuff like ::start() > WebCore/storage/IDBTransactionBackendImpl.cpp:146 > + queue.swap(m_taskQueue); Why do we need to swap like this? If stuff is added, it'll be at the end of the list...and you just keep grabbing the first item. So I think you're ok. Oh...but then I guess you'll need to make the above logic not reschedule another timer......so I guess this is ok. > WebCore/storage/IDBTransactionBackendImpl.cpp:151 > + m_pendingEvents++; should this happen before we call performTask? Otherwise it's possible that the task should call didCompleteTaskEvents But, in general, this seems fairly fragile. Can we do it any better? Maybe keep a list of transactions with pending tasks and then call didCompleteTaskEvents at the end of the while loop on each one? > WebCore/storage/IDBTransactionBackendImpl.h:62 > + IDLE, I believe this is not webkit style. (We do it this way only when matching IDLs.) maybe s/IDLE/Unused/ or NotStarted? > WebCore/storage/IDBTransactionBackendImpl.h:73 > RefPtr<DOMStringList> m_objectStoreNames; This is a long list. Maybe add some newlines in to logically group things together? > WebCore/storage/IDBTransactionCoordinator.cpp:61 > + RefPtr<IDBTransactionBackendInterface> transaction = m_pendingTransactions.get(id); ewwww!!!!!!! we should not pass around transaction interfaces. Lets stick with either ids (when something needs to be passed outside of other backendImpls) or IDBTransactionBackendImpls. note that you just went tot he effort to lookup the _interface_ object a moment ago in the proxy layer > WebCore/storage/IDBTransactionCoordinator.cpp:62 > + ASSERT(transaction); dont' do this when the next instrction would crash anyway > WebCore/storage/IDBTransactionCoordinator.cpp:69 > ASSERT(transaction); not needed...you'll crash on the next line anyway > WebCore/storage/IDBTransactionCoordinator.cpp:71 > + m_runningTransactions.get(transaction->id())->didCompleteTaskEvents(); ewwww we should not pass around transaction interfaces. Lets stick with either ids (when something needs to be passed outside of other backendImpls) or IDBTransactionBackendImpls. > WebCore/storage/IDBTransactionCoordinator.cpp:77 > + ASSERT(transaction); not needed > WebCore/storage/IDBTransactionCoordinator.cpp:83 > + // Add to started transactions list maybe a newline above this? whenever i see code blocks of over 4-5 instructions, i start looking at where there are natural groupings of operations and tend to split there. maybe i'm overly nit-picky tho? period at end > WebCore/storage/IDBTransactionCoordinator.cpp:108 > +IDBTransactionBackendInterface* IDBTransactionCoordinator::transaction(int transactionID) maybe transactionFromId? I don't care much > WebCore/storage/IDBTransactionCoordinator.cpp:125 > + // that's ready to run. For now we only have a single running no need to wrap at 80 chars :-) 2 lines seems ok for somethign this long tho > WebCore/storage/IDBTransactionCoordinator.cpp:138 > +}; extra space > WebKit/chromium/public/WebIDBTransaction.h:62 > + virtual operator WebCore::IDBTransactionBackendInterface*() const = 0; I'm not sure we need this here....the callers should be dealing only with the impl object, right? > WebKit/chromium/src/IDBDatabaseProxy.h:53 > + virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0); I don't think you need the default param here > WebKit/chromium/src/IDBFactoryBackendProxy.cpp:74 > + ids[i] = transactions.at(i)->id(); Hm. I guess this works, but I had kinda envisioned doing this at the Chromium level....but i guess i can't come up with any disadvantages to doing it this way and it does seem simpler. ok. > WebKit/chromium/src/IDBObjectStoreProxy.cpp:77 > + IDBTransactionBackendProxy* transactionProxy = static_cast<IDBTransactionBackendProxy*>(transaction); Add a comment explaining why this is safe maybe? > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72 > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>) Lets get rid of it from the interface then. > WebKit/chromium/src/IDBTransactionBackendProxy.h:52 > + WebKit::WebIDBTransaction* toWebIDBTransaction() const { return m_webIDBTransaction.get(); } this isn't really converting it, so "to" should probably not be here > WebKit/chromium/src/WebIDBFactoryImpl.cpp:69 > + WTF::Vector<WebCore::IDBTransactionBackendInterface*> transactions(pendingIDs.size()); just do a using namespace webocre at the top? > WebKit/chromium/src/WebIDBTransactionImpl.h:-36 > -namespace WebCore { class IDBTransactionBackendInterface; } I believe a using WebCore is OK to do at the top here. > WebKit/chromium/src/WebIDBTransactionImpl.h:50 > + virtual operator WebCore::IDBTransactionBackendInterface*() const; needn't be virtual...maybe just make it inline? Not sure if it makes sense to do this as an operator. The more I deal with operators, the more I've not been super impressed by them. Maybe just getWebCoreBackend? webCoreBackend...or just backend()?
Comment on attachment 68163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68163&action=review >> WebCore/storage/IDBObjectStoreBackendImpl.cpp:84 >> + if (transaction) { > > FIXME: This is only necessary until .... oh.....i see......i guess 2 methods is cleaner > WebCore/storage/IDBObjectStoreBackendImpl.cpp:86 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_ALLOWED_ERR, "Get must be called in the context of a transaction.")); crap...at some point this will need to be thrown synchronously....but fine to leave for now.
hanks for the review. All fixed except: (In reply to comment #19) > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:40 > > + m_transactions = new Vector<IDBTransactionBackendInterface*>(); > > NOOOOOO manual memory management! > > Why make the vector like this? at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to? > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:56 > > + delete m_transactions; > > NOOOOO > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:68 > > + delete m_transactions; > > NOOOOOO > > > WebCore/storage/IDBPendingTransactionMonitor.h:58 > > + static Vector<IDBTransactionBackendInterface*>* m_transactions; > > monitor is in browser process, right? if so, it should have BackendImpl pointers....or maybe even refs? > No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help. > > WebCore/storage/IDBRequest.cpp:174 > > + if (m_transaction.get()) { > > no .get() > > > WebCore/storage/IDBRequest.cpp:180 > > + m_transaction.clear(); > > why clear it? i know we clear objects in a few places, but i'm wondering if it's really worth bothering > We don't need the transaction anymore after this but the request object will only be released later when GC happens. I was therefore opting for clearing the pointer so that we don't needlessly keep the transaction object alive. > > WebCore/storage/IDBRequest.h:97 > > + RefPtr<IDBTransactionBackendInterface> m_transaction; > > store an id here...and anywhere else other than backend impls > > > WebCore/storage/IDBTransaction.cpp:70 > > + throwError(NOT_SUPPORTED_ERR); > > as i mentioned in the other thread, i think this is the wrong error...we should use some idb error. unknown if there isn't one specced yet (and file a spec bug) > Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec? > > WebCore/storage/IDBTransactionBackendImpl.cpp:68 > > + if (m_state == IDLE) > > some of your methods here can always be called (like abort) and bail if it's not in a state where that makes sense...and some like start assume you only call it when it makes sense. Maybe you should be consistent? > The methods that bail are the ones called from JS side, like abort or scheduleTask (a result of IDBObjectStore::get/put/etc). We don't control when those are called so we bail when there is nothing to do or we need to report an error. The methods that don't bail are part of the internal state machine logic of the transaction and the transaction coordinator. There I have a bunch of asserts to make sure the state is always as expected. > > WebCore/storage/IDBTransactionBackendImpl.cpp:146 > > + queue.swap(m_taskQueue); > > Why do we need to swap like this? If stuff is added, it'll be at the end of the list...and you just keep grabbing the first item. So I think you're ok. > > Oh...but then I guess you'll need to make the above logic not reschedule another timer......so I guess this is ok. > Yes. > > WebCore/storage/IDBTransactionBackendImpl.cpp:151 > > + m_pendingEvents++; > > should this happen before we call performTask? Otherwise it's possible that the task should call didCompleteTaskEvents > That should not happen synchronously. The events complete asynchronously. > But, in general, this seems fairly fragile. Can we do it any better? Maybe keep a list of transactions with pending tasks and then call didCompleteTaskEvents at the end of the while loop on each one? > Added an assert in the IDBRequest dtor to check that the events fired for the associated transaction when the IDBRequest is destroyed. > > > WebKit/chromium/public/WebIDBTransaction.h:62 > > + virtual operator WebCore::IDBTransactionBackendInterface*() const = 0; > > I'm not sure we need this here....the callers should be dealing only with the impl object, right? > We're passing a WebIDBTransaction const ref to the WebIDBObjectStore::get() and such. The get() method needs to pass an IDBTransactionBackendInterface ptr to IDBObjectStoreBackendImpl::get(). It therefore needs to obtain an IDBTransactionBackendInterface from a WebIDBTransaction. This operator achieves that. > > WebKit/chromium/src/IDBDatabaseProxy.h:53 > > + virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0); > > I don't think you need the default param here > We're overriding a method from the interface. I think we do? > > > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72 > > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>) > > Lets get rid of it from the interface then. > If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is?
(In reply to comment #21) > hanks for the review. All fixed except: > > (In reply to comment #19) > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:40 > > > + m_transactions = new Vector<IDBTransactionBackendInterface*>(); > > > > NOOOOOO manual memory management! > > > > Why make the vector like this? at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to? > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:56 > > > + delete m_transactions; > > > > NOOOOO > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:68 > > > + delete m_transactions; > > > > NOOOOOO > > > > > WebCore/storage/IDBPendingTransactionMonitor.h:58 > > > + static Vector<IDBTransactionBackendInterface*>* m_transactions; > > > > monitor is in browser process, right? if so, it should have BackendImpl pointers....or maybe even refs? > > > > No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help. Oh yeah. I remember now. The reason we can't use an own ptr is because we can't have objects with constructors the global scope. Maybe we could make a static function with an own ptr that owns this though? That would be replaced by some TLS based solution in the future. > > > WebCore/storage/IDBRequest.cpp:174 > > > + if (m_transaction.get()) { > > > > no .get() > > > > > WebCore/storage/IDBRequest.cpp:180 > > > + m_transaction.clear(); > > > > why clear it? i know we clear objects in a few places, but i'm wondering if it's really worth bothering > > > > We don't need the transaction anymore after this but the request object will only be released later when GC happens. I was therefore opting for clearing the pointer so that we don't needlessly keep the transaction object alive. Actually it does seem like transactions could introduce some hairy cycles. Probably a good idea. > > > WebCore/storage/IDBRequest.h:97 > > > + RefPtr<IDBTransactionBackendInterface> m_transaction; > > > > store an id here...and anywhere else other than backend impls > > > > > WebCore/storage/IDBTransaction.cpp:70 > > > + throwError(NOT_SUPPORTED_ERR); > > > > as i mentioned in the other thread, i think this is the wrong error...we should use some idb error. unknown if there isn't one specced yet (and file a spec bug) > > > > Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec? Please start a mailing list thread on this. Do we have issues with needing strings then? (If so, let's do this in another patch maybe?) Do we test this case in the layout tests? > > > WebCore/storage/IDBTransactionBackendImpl.cpp:151 > > > + m_pendingEvents++; > > > > should this happen before we call performTask? Otherwise it's possible that the task should call didCompleteTaskEvents > > > > That should not happen synchronously. The events complete asynchronously. I still think you should move it. > > > WebKit/chromium/public/WebIDBTransaction.h:62 > > > + virtual operator WebCore::IDBTransactionBackendInterface*() const = 0; > > > > I'm not sure we need this here....the callers should be dealing only with the impl object, right? > > > > We're passing a WebIDBTransaction const ref to the WebIDBObjectStore::get() and such. The get() method needs to pass an IDBTransactionBackendInterface ptr to IDBObjectStoreBackendImpl::get(). It therefore needs to obtain an IDBTransactionBackendInterface from a WebIDBTransaction. This operator achieves that. Gotcha....well see my other comment for the operator stuff you added. I feel as though a virtual method would be much more clear. > > > WebKit/chromium/src/IDBDatabaseProxy.h:53 > > > + virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0); > > > > I don't think you need the default param here > > > > We're overriding a method from the interface. I think we do? 99% sure we don't and shouldn't. > > > > > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72 > > > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>) > > > > Lets get rid of it from the interface then. > > > > If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is? We have many cases where backend impls hold references to other backend impls. I don't see what's different here. I'm pretty sure the plumbing should be trivial.
(In reply to comment #22) > (In reply to comment #21) > > hanks for the review. All fixed except: > > > > (In reply to comment #19) > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:40 > > > > + m_transactions = new Vector<IDBTransactionBackendInterface*>(); > > > > > > NOOOOOO manual memory management! > > > > > > Why make the vector like this? at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to? > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:56 > > > > + delete m_transactions; > > > > > > NOOOOO > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:68 > > > > + delete m_transactions; > > > > > > NOOOOOO > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.h:58 > > > > + static Vector<IDBTransactionBackendInterface*>* m_transactions; > > > > > > monitor is in browser process, right? if so, it should have BackendImpl pointers....or maybe even refs? > > > > > > > No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help. > > Oh yeah. I remember now. The reason we can't use an own ptr is because we can't have objects with constructors the global scope. Maybe we could make a static function with an own ptr that owns this though? That would be replaced by some TLS based solution in the future. > Hmm, let's chat about this tomorrow. Still not sure it's a great improvement. > > > > > > > WebCore/storage/IDBTransaction.cpp:70 > > > > + throwError(NOT_SUPPORTED_ERR); > > > > > > as i mentioned in the other thread, i think this is the wrong error...we should use some idb error. unknown if there isn't one specced yet (and file a spec bug) > > > > > > > Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec? > > Please start a mailing list thread on this. Do we have issues with needing strings then? (If so, let's do this in another patch maybe?) Do we test this case in the layout tests? > Ok, will do, > > > > > > > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72 > > > > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>) > > > > > > Lets get rid of it from the interface then. > > > > > > > If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is? > > We have many cases where backend impls hold references to other backend impls. I don't see what's different here. I'm pretty sure the plumbing should be trivial. But the object store backend cannot possibly hold a reference to the transaction backend, since multiple transactions can be associated with the same object store backend at the same time. So the transaction needs to be stored in the frontend object and passed into the object store's methods as needed. However, the frontend can only store transaction interface pointers, as Chromium uses proxy objects in the renderer. And since it's the frontend object that supplies the transaction parameter to the object store's backend methods, it follows that these methods can only get an interface pointer and not an impl pointer. Or perhaps I am missing something?
Created attachment 68476 [details] Patch
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > hanks for the review. All fixed except: > > > > > > (In reply to comment #19) > > > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:40 > > > > > + m_transactions = new Vector<IDBTransactionBackendInterface*>(); > > > > > > > > NOOOOOO manual memory management! > > > > > > > > Why make the vector like this? at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to? > > > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:56 > > > > > + delete m_transactions; > > > > > > > > NOOOOO > > > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.cpp:68 > > > > > + delete m_transactions; > > > > > > > > NOOOOOO > > > > > > > > > WebCore/storage/IDBPendingTransactionMonitor.h:58 > > > > > + static Vector<IDBTransactionBackendInterface*>* m_transactions; > > > > > > > > monitor is in browser process, right? if so, it should have BackendImpl pointers....or maybe even refs? > > > > > > > > > > No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help. > > > > Oh yeah. I remember now. The reason we can't use an own ptr is because we can't have objects with constructors the global scope. Maybe we could make a static function with an own ptr that owns this though? That would be replaced by some TLS based solution in the future. > > > > Hmm, let's chat about this tomorrow. Still not sure it's a great improvement. Just leave it as is. > > > > > > > > > WebCore/storage/IDBTransaction.cpp:70 > > > > > + throwError(NOT_SUPPORTED_ERR); > > > > > > > > as i mentioned in the other thread, i think this is the wrong error...we should use some idb error. unknown if there isn't one specced yet (and file a spec bug) > > > > > > > > > > Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec? > > > > Please start a mailing list thread on this. Do we have issues with needing strings then? (If so, let's do this in another patch maybe?) Do we test this case in the layout tests? > > > > Ok, will do, > > > > > > > > > > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72 > > > > > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>) > > > > > > > > Lets get rid of it from the interface then. > > > > > > > > > > If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is? > > > > We have many cases where backend impls hold references to other backend impls. I don't see what's different here. I'm pretty sure the plumbing should be trivial. > > > But the object store backend cannot possibly hold a reference to the transaction backend, since multiple transactions can be associated with the same object store backend at the same time. So the transaction needs to be stored in the frontend object and passed into the object store's methods as needed. > > However, the frontend can only store transaction interface pointers, as Chromium uses proxy objects in the renderer. And since it's the frontend object that supplies the transaction parameter to the object store's backend methods, it follows that these methods can only get an interface pointer and not an impl pointer. Or perhaps I am missing something? No, you're right. OK, leave it as is.
Comment on attachment 68476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68476&action=review few comments until you can get me the re-based version with the test changes....close tho > WebCore/storage/IDBObjectStoreBackendImpl.h:76 > + template <class T, class Method, class Param1, class Param2> Could this stuff just all go in the .cpp? > WebCore/storage/IDBTransactionCoordinator.h:70 > + HashMap<int, RefPtr<IDBTransactionBackendImpl> > m_transactions; This is a little be weird, but probably cleaner than anything else I can think of (like having transactions self ref). > WebCore/storage/IDBTransactionCoordinator.h:72 > + HashSet<IDBTransactionBackendImpl*> m_pendingTransactions; Is this still necessary?
Created attachment 68514 [details] Patch
Comment on attachment 68514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68514&action=review > LayoutTests/storage/indexeddb/objectstore-removeobjectstore.html:104 > + transaction.onabort = unexpectedErrorCallback; do abort events have messages and codes? if so, this won't work > WebCore/storage/IDBTransactionCoordinator.cpp:57 > + m_startedTransactions.add(transaction); assert it's in m_transactions maybe? > WebCore/storage/IDBTransactionCoordinator.cpp:86 > + m_runningTransactions.add(transaction); assert it's in m_transaction still?
r=me
Committed r68138: <http://trac.webkit.org/changeset/68138>
http://trac.webkit.org/changeset/68138 might have broken Chromium Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/68136 http://trac.webkit.org/changeset/68137 http://trac.webkit.org/changeset/68138