There's a way to access an IndexedDB from a worker thread with a synchronous API. We should support it.

Actually, per spec, workers are supposed to have access to both the sync and the async API.

I note that in the current worker setup doing this will be *excruciatingly* painful.

Why so painful? Please tell us more.

There is no XPConnect in workers, so all of the bindings must be written manually using JSAPI.

(The long term plan here is to have tools that automatically generate that goop, but those don't exist yet.)

I've started to think that we should bring back xpconnect-workers, and re-enable JS API workers once it is easier hack them. Right now adding any features to workers is close to insanely-hard.

Please file a separate bug on that. As for this bug. We should definitely fix it, but I think it's higher priority that we get the implementation finished and un-prefixed on the main thread first. This definitely won't be something we'll work on this quarter given the complexity involved.

Also, let's remove from tracker bug 687337. That one is about getting our main thread async API up to spec.

Are there separate bugs for the async and sync api being available from workers?

Andrea, do we have bugs for the sync API?

I don't think so. Not yet.

Are there plans for implementing Async or Sync IndexedDB api in web workers in FF soon?

(In reply to Deni from comment #13) > Are there plans for implementing Async or Sync IndexedDB api in web workers > in FF soon? We may try for later this calendar year but it's up in the air ATM.

I filed bug 798875 for synchronous IDB API, there's a guy who is considering to do it as his diploma thesis, but it hasn't been officially approved yet (by the university).

I'm not so much interested in the sync APIs, but having access to the async APIs from workers (in addition to the main thread) would be a killer feature for a lot of apps that are trying to shift expensive storage/processing operations off the main thread, such as games and game-like apps that need to maintain 60fps. Also, the MDN page on IndexedDB says that the async API is available from workers even though it's currently not in FF (but is in Chrome) - it should probably be called out that it's not yet available everywhere.

+1 to Ben Vanik's comment above, the sync API has little benefit for WebGL use cases but just providing the same IDB async API in a worker context would be so amazingly awesome for making games. The other big missing piece is also to implement Transferable Objects between workers and the main context (http://updates.html5rocks.com/2011/12/Transferable-Objects-Lightning-Fast)

(In reply to max from comment #17) > The other big missing piece > is also to implement Transferable Objects between workers and the main > context > (http://updates.html5rocks.com/2011/12/Transferable-Objects-Lightning-Fast) Not about this bug at all, but Bug 720083 was fixed for FF18.

Andrea, why did you make this bug depend on bug 783190 ?

(In reply to Jan Varga [:janv] from comment #19) > Andrea, why did you make this bug depend on bug 783190 ? This is a mistake and should be removed: the sync API is an abstraction on-top of the of async API and should not be a blocker for this enhancement. Andrea, if I'm wrong, would you mind correcting me? :)

This is a helper class that gives access to QuotaManager mainthread only methods from workers.

Still needs to have bug 914762 landed.

Comment on attachment 8345387 [details] [diff] [review] patch 1 - TransactionThreadPool without DOM elements. Review of attachment 8345387 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ +791,5 @@ > > +void > +IDBDatabase::RegisterTransaction(IDBTransaction* aTransaction) > +{ > + mTransactions.PutEntry(aTransaction); Let's first assert that we're on the 'correct' thread, then that aTransaction is non-null, and then assert that mTransactions does not already contain aTransaction. Same below for Unregister (except reverse the Contains() assertion). @@ +802,5 @@ > +} > + > +static > +PLDHashOperator AbortTransactionsEnumerator(nsPtrHashKey<IDBTransaction>* aTransaction, > + void* aNonUsed) Nit: Return type on its own line, and move this up into the anonymous namespace at the top of the file @@ +815,5 @@ > + > +void > +IDBDatabase::AbortTransactions() > +{ > + mTransactions.EnumerateEntries(AbortTransactionsEnumerator, nullptr); Assert correct thread here. ::: dom/indexedDB/IDBDatabase.h @@ +267,5 @@ > mozilla::dom::ContentParent* mContentParent; > > nsRefPtr<mozilla::dom::quota::Client> mQuotaClient; > > + nsTHashtable<nsPtrHashKey<IDBTransaction>> mTransactions; I think this needs to be nsRefPtrHashKey... ::: dom/indexedDB/IDBTransaction.cpp @@ +114,5 @@ > transaction->mDatabaseInfo = aDatabase->Info(); > transaction->mObjectStoreNames.AppendElements(aObjectStoreNames); > transaction->mObjectStoreNames.Sort(); > > + aDatabase->RegisterTransaction(transaction); You should only register once the transaction has been sent to the pool. There are a few early returns here that could leave a transaction registered incorrectly here. @@ +180,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > SetIsDOMBinding(); > + > + mId = TransactionThreadPool::GetUniqueId(); Let's set this in CreateInternal with the rest of the members? ::: dom/indexedDB/TransactionThreadPool.cpp @@ +128,5 @@ > > +uint64_t > +TransactionThreadPool::GetUniqueId() > +{ > + return ++gUniqueTransactionId; This is only safe as long as we're only called on a single thread. Need some assertions here. ::: dom/indexedDB/TransactionThreadPool.h @@ +37,5 @@ > static TransactionThreadPool* Get(); > > static void Shutdown(); > > + static uint64_t GetUniqueId(); Nit: NextTransactionId? @@ +42,5 @@ > + > + nsresult Dispatch(const uint64_t& aTransactionId, > + const nsACString& aDatabaseId, > + const nsTArray<nsString>& aObjectStoreNames, > + const uint16_t aMode, Nit: here and below, don't pass const refs for ints, or const ints for that matter. @@ +47,5 @@ > nsIRunnable* aRunnable, > bool aFinish, > nsIRunnable* aFinishRunnable); > > + nsresult Dispatch(const uint64_t& aTransactionId, Hm, I don't see why you need two Dispatch methods? @@ +85,5 @@ > + > + uint64_t mTransactionId; > + const nsCString mDatabaseId; > + const nsTArray<nsString> mObjectStoreNames; > + uint16_t mMode; Hm, does the queue really need to hold on to this information? @@ +104,5 @@ > { > MOZ_COUNT_CTOR(TransactionInfo); > > + transactionId = aTransactionId; > + databaseId = aDatabaseId; These should go in an initializer list @@ +151,5 @@ > { > MOZ_COUNT_DTOR(DatabaseTransactionInfo); > } > > + typedef nsClassHashtable<nsUint64HashKey, TransactionInfo > Nit: not your fault, but kill that space before > @@ +192,5 @@ > + TransactionQueue& GetQueueForTransaction( > + const uint64_t& aTransactionId, > + const nsACString& aDatabaseId, > + const nsTArray<nsString>& aObjectStoreNames, > + const uint16_t aMode); These can both take a TransactionInfo* can't they?

> > + > > + uint64_t mTransactionId; > > + const nsCString mDatabaseId; > > + const nsTArray<nsString> mObjectStoreNames; > > + uint16_t mMode; > > Hm, does the queue really need to hold on to this information? They are used for FinishTransactionRunnable(). > @@ +192,5 @@ > > + TransactionQueue& GetQueueForTransaction( > > + const uint64_t& aTransactionId, > > + const nsACString& aDatabaseId, > > + const nsTArray<nsString>& aObjectStoreNames, > > + const uint16_t aMode); > > These can both take a TransactionInfo* can't they? GetQueueForTransaction creates TransactionInfo if it doesn't exist yet. Why should it receive one? For avoiding so many arguments in this method?

With the patches in bug 949682, we could migrate IDBDatabase.objectStoreNames and IDBObjectStore.indexNames to be of type "[Pure, Cached, Frozen] readonly attribute sequence<DOMString>". That would mean we don't have to make DOMStringList work in workers. Not sure if that would simplify our lives here or not.

Comment on attachment 8346577 [details] [diff] [review] patch 1 - TransactionThreadPool without DOM elements. Review of attachment 8346577 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ +803,5 @@ > > +void > +IDBDatabase::RegisterTransaction(IDBTransaction* aTransaction) > +{ > + MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!"); This will need to be updated to be the "correct thread" some time soon, right? It won't always be the main thread.

rebased

Comment on attachment 8350102 [details] [diff] [review] transaction.patch Review of attachment 8350102 [details] [diff] [review]: ----------------------------------------------------------------- I'm trying to remove nsIFileStorage and possibly do the same what this patch does to FileService, but I think I found at least one problem in this patch. ::: dom/indexedDB/TransactionThreadPool.cpp @@ +466,1 @@ > dbTransactionInfo->transactions.EnumerateRead(FindTransaction, &info); You already found out that there are some transactions for this database id and the enumeration was there to find concrete IDBDatabase, but since you removed IDBDatabase, you don't need to enumerate. However, I'm not sure if this is actually right. HasTransactionsForDatabase() is called by QuotaManager::HasOpenTransactions(nsPIDOMWindow*) and that gets databases for specific window (not for a database id), so the method would now return true if some other window has databases for the database id.

Heys guys, just want to let you know, that indexedDB in worker is so important for seamless webgl experience. I hope you can bring this into the amazing fox one day!

(In reply to Markus Siebeneicher from comment #31) > Heys guys, just want to let you know, that indexedDB in worker is so > important for seamless webgl experience. I hope you can bring this into the > amazing fox one day! +100!

This has sorta become a meta bug. Development on this project is happening in the jamun project branch (http://hg.mozilla.org/projects/jamun).

What's the status of this bug? Is it actively being worked on? Anything I can do to help?

Yep, we're on it, it's top priority for several of us.

I guess bent is working on this.

(This is just moving code, no functional changes)

For those following along, I hope to land this early in the next cycle so that gaia folks can develop with indexedDB in workers for FxOS 2.2.

\o/

Comment on attachment 8528820 [details] [diff] [review] Part 1: WebIDL stubs and pref, v1 Review of attachment 8528820 [details] [diff] [review]: ----------------------------------------------------------------- Actually, Andrea, do you want to review this one?

Kyle, feel free to bounce reviews to anyone else you think could do them.

Comment on attachment 8528820 [details] [diff] [review] Part 1: WebIDL stubs and pref, v1 Review of attachment 8528820 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +133,5 @@ > > mozilla::Atomic<bool> gInitialized(false); > mozilla::Atomic<bool> gClosed(false); > mozilla::Atomic<bool> gTestingMode(false); > +Atomic<bool> gExperimentalFeaturesEnabled(false); mozilla::Atomic? or remove mozilla:: from the others. Actually, drop mozilla:: from the others. ::: dom/webidl/DOMError.webidl @@ +10,5 @@ > * liability, trademark and document use rules apply. > */ > > [Constructor(DOMString name, optional DOMString message = ""), > + Exposed=(Window,Worker,System)] Ok. This is fine but, DOMError has 3 constructors, 1 for JS and 2 for C++ only. One of the C++ constructor uses NS_GetNameAndMessageForDOMNSResult that is main-thread only. Add an assertion there in order to prevent the use of this ctor out of main-thread. ::: dom/webidl/DOMStringList.webidl @@ +14,1 @@ > interface DOMStringList { About DOMStringList, we have several subclasses and we don't want to expose all of them to workers. In order to avoid wrong future uses, can you add an assert to the virtual method EnsureFresh of subclasses (PropertyStringList and nsDOMStyleSheetSetList) ? ::: dom/workers/WorkerScope.h @@ +39,5 @@ > class WorkerGlobalScope : public DOMEventTargetHelper, > public nsIGlobalObject > { > + typedef mozilla::dom::indexedDB::IDBFactory IDBFactory; > + mozilla::dom:: should not be needed.

Comment on attachment 8528822 [details] [diff] [review] Part 2: Refactor 3rd party checks, v1 Review of attachment 8528822 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +259,5 @@ > using namespace mozilla::dom; > using namespace mozilla::dom::ipc; > using mozilla::TimeStamp; > using mozilla::TimeDuration; > +using mozilla::dom::indexedDB::IDBFactory; alphabetic order? ::: dom/indexedDB/IDBFactory.cpp @@ +143,5 @@ > + } > + > + if (NS_WARN_IF(NS_FAILED(rv))) { > + if (rv == NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR) { > + IDB_REPORT_INTERNAL_ERR(); If you want this here, then remove it in AllowedForWindowInternal. @@ +273,5 @@ > + nsresult rv = AllowedForWindowInternal(aWindow, getter_AddRefs(principal)); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return false; > + } > + In theory rv can be NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR and you want to report the error using IDB_REPORT_INTERNAL_ERR(). @@ +293,5 @@ > + } > + > + nsIDocument* document = aWindow->GetExtantDoc(); > + if (document->GetSandboxFlags() & SANDBOXED_ORIGIN) { > + return NS_ERROR_DOM_SECURITY_ERR; Why don't we have IDB_REPORT_INTERNAL_ERR() here and in the other error conditions? @@ +311,5 @@ > + } > + > + bool isNullPrincipal; > + if (NS_WARN_IF(NS_FAILED(principal->GetIsNullPrincipal(&isNullPrincipal))) || > + isNullPrincipal) { Maybe we want to NS_WARN_IF(isNullPrincipal) ?

Comment on attachment 8528826 [details] [diff] [review] Part 4: Fix wrapping of binding objects, v1 Review of attachment 8528826 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsChild.cpp @@ +354,5 @@ > private: > + template <class T> > + typename EnableIf<IsSame<T, IDBDatabase>::value || > + IsSame<T, IDBCursor>::value || > + IsSame<T, IDBMutableFile>::value, indentation

Comment on attachment 8528827 [details] [diff] [review] Part 5: Fix error events fired at global scope, v1 Review of attachment 8528827 [details] [diff] [review]: ----------------------------------------------------------------- r- for the innerWindowID ::: dom/indexedDB/IDBRequest.h @@ +118,5 @@ > + GetErrorAfterResult() const > +#ifdef DEBUG > + ; > +#else > + { Do we care to AssertIsOnOwningThread() ? ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +423,2 @@ > > + nsRefPtr<DOMError> error = request->GetErrorAfterResult(); This maybe is not so important, but in theory we use 'GetFoobar()' when the return value can be null. Otherwise we use Foobar(). Can we rename GetErrorAfterResult() to 'ErrorAfterResult()' ? Then MOZ_ASSERT(error); @@ +490,5 @@ > + category.AssignLiteral("chrome "); > + } else { > + category.AssignLiteral("content "); > + } > + category.AppendLiteral("javascript"); category should contain 'indexedDB' somewhere. @@ +519,5 @@ > + /* aSourceLine */ EmptyString(), > + init.mLineno, > + /* aColumnNumber */ 0, > + nsIScriptError::errorFlag, > + category.get()))); In theory, you can have the window inner ID in workers too taking it from the parent window. This will be perfect for debugging, because the webConsole will log this message correctly. If we have take the correct window inner ID from the window when we create the factory, we can use this value.

(In reply to Andrea Marchesini (:baku) from comment #55) > One of the C++ constructor uses NS_GetNameAndMessageForDOMNSResult that is > main-thread only. Add an assertion there in order to prevent the use of this > ctor out of main-thread. I looked at this and it didn't actually look like NS_GetNameAndMessageForDOMNSResult is main-thread only so I didn't add the assertion... Am I missing something? > mozilla::dom:: should not be needed. I kinda prefer it for explicit typedefs like this though. Any objection to leaving it?

(In reply to Andrea Marchesini (:baku) from comment #56) > alphabetic order? It seems ok to me since we're entering another namespace... Is there something different you'd suggest? > In theory rv can be NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR and you want to > report the error using IDB_REPORT_INTERNAL_ERR(). No, the goal is that AllowedForWindow shouldn't log anything in the console ever. I fixed the (accidental) place where it used to do that via AllowedForWindowInternal. CreateForWindow does log stuff. > Why don't we have IDB_REPORT_INTERNAL_ERR() here and in the other error > conditions? The deal with this is that the error the page sees is UnknownErr and that's incredibly not useful, so we add more info to the error console whenever we throw that one specific code. Other errors like SecurityErr are spec'd. > Maybe we want to NS_WARN_IF(isNullPrincipal) ? I didn't add this because I don't think it's useful.

(In reply to Andrea Marchesini (:baku) from comment #58) > Do we care to AssertIsOnOwningThread() ? This gets asserted in the DEBUG version, but there's no point in asserting for the non-DEBUG version. > Can we rename GetErrorAfterResult() to 'ErrorAfterResult()' ? It still returns null sometimes. > category should contain 'indexedDB' somewhere. No, this is somehow special for the devtools stuff, there's a hardcoded list of stuff it understands. > If we have take the correct window inner ID from the window when we create > the factory, we can use this value. Fixed.

Comment on attachment 8528828 [details] [diff] [review] Part 6: Hook transaction lifetime to the worker event loop, v1 Review of attachment 8528828 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBRequest.cpp @@ +544,5 @@ > +IDBOpenDBRequest::NoteComplete() > +{ > + AssertIsOnOwningThread(); > + MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerFeature); > + write a comment about that nullifying mWorkerFeature we remove the feature in its DTOR. ::: dom/indexedDB/IDBTransaction.cpp @@ +69,4 @@ > } // anonymous namespace > > +class IDBTransaction::WorkerFeature MOZ_FINAL > + : public mozilla::dom::workers::WorkerFeature remove mozilla::dom::workers:: @@ +71,5 @@ > +class IDBTransaction::WorkerFeature MOZ_FINAL > + : public mozilla::dom::workers::WorkerFeature > +{ > + WorkerPrivate* mWorkerPrivate; > + IDBTransaction* mTransaction; Maybe add a comment saying that the feature is kept alive by this IDBTransaction so we don't need to play with the refcounting. ::: dom/workers/WorkerPrivate.cpp @@ +2081,5 @@ > #endif > { > } > > +struct WorkerPrivate::PreemptingRunnable MOZ_FINAL NIT: I'm not a native english speaker, but this is called 'runnable', but actually it is not a runnable. What about a different name? @@ +4390,5 @@ > + pending->mRunnable.swap(preemptingRunnable.mRunnable); > + pending->mRecursionDepth = preemptingRunnable.mRecursionDepth; > + } > + } > + what about: for (uint32_t index = 0; ; index < mPreemptingRunnables.Length();) { PreemptingRunnable& preemptingRunnable = mPreemptingRunnables[index]; if (preemptingRunnable.mRecursionDepth == aRecursionDepth) { preemtingRunnable.mRunnable->Run(); mPreemptingRunnables.RemoveAt(index); } else { ++index; } } instead allocating new memory... ? @@ +4423,5 @@ > + preemptingRunnable->mRecursionDepth = recursionDepth ? recursionDepth - 1 : 0; > + > + // Ensure that we have a pending event so that the runnable will be guaranteed > + // to run. > + if (mPreemptingRunnables.Length() == 1 && !NS_HasPendingEvents(mThread)) { remove mPreemtingRunnables.Length() == 1. Don't we have to add a dummy event in any case if we don't have a pending one?

Comment on attachment 8528829 [details] [diff] [review] Part 7: Fix blobs on workers, v1 Review of attachment 8528829 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ +67,5 @@ > const char kMemoryPressureObserverTopic[] = "memory-pressure"; > const char kWindowObserverTopic[] = "inner-window-destroyed"; > > +class CancelableRunnableWrapper MOZ_FINAL > + : public nsICancelableRunnable can you use nsCancelableRUnnable instead? @@ +78,5 @@ > + { > + MOZ_ASSERT(aRunnable); > + } > + > + NS_DECL_THREADSAFE_ISUPPORTS then remove this line and NS_DECL_NSIRUNNABLE && NS_DECL_NSICANCELABLERUNNABLE. @@ +1344,5 @@ > { > return IDBDatabaseBinding::Wrap(aCx, this); > } > > +NS_IMPL_ISUPPORTS(CancelableRunnableWrapper, nsIRunnable, nsICancelableRunnable) you can remove this line. @@ +1348,5 @@ > +NS_IMPL_ISUPPORTS(CancelableRunnableWrapper, nsIRunnable, nsICancelableRunnable) > + > +NS_IMETHODIMP > +CancelableRunnableWrapper::Run() > +{ In theory mRunnable should exist here because Run() cannot be called after mRunnable. So probably this code should be: MOZ_ASSERT(mRunnable); nsCOMPtr<nsIRunnable> runnable; mRunnable.swap(runnable); return runnable->Run(); correct? ::: dom/workers/RuntimeService.cpp @@ +1686,5 @@ > + if (!BackgroundChild::GetForCurrentThread()) { > + nsRefPtr<BackgroundChildCallback> callback = new BackgroundChildCallback(); > + if (!BackgroundChild::GetOrCreateForCurrentThread(callback)) { > + MOZ_CRASH("Unable to connect PBackground actor for the main thread!"); > + } I don't think this is enough to be 100% sure that we can call MOZ_ASSERT(BackgroundChild::GetForCurrenThread()) somewhere else. Correct?

Comment on attachment 8528833 [details] [diff] [review] Part 11: Fix the aborted transaction warning for workers, v1 Review of attachment 8528833 [details] [diff] [review]: ----------------------------------------------------------------- I suspect that this patch cannot be applied on top of patch 5. ::: dom/indexedDB/IDBDatabase.cpp @@ +177,5 @@ > }; > > } // anonymous namespace > > +class IDBDatabase::LogWarningRunnable MOZ_FINAL wait... this is part of patch 5. Correct?

Comment on attachment 8528835 [details] [diff] [review] Part 12: Fix small issues revealed by tests, v1 Review of attachment 8528835 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsChild.cpp @@ +2485,5 @@ > + return NS_ERROR_UNEXPECTED; > + } > + > + // This must always run to clean up our state. > + Run(); return Run(); ?

Rebased part 11 over changes from earlier review notes.

Comment on attachment 8528828 [details] [diff] [review] Part 6: Hook transaction lifetime to the worker event loop, v1 Review of attachment 8528828 [details] [diff] [review]: ----------------------------------------------------------------- Do we have tests that test the ordering of IDB events in the presence of sync XHR/etc? ::: dom/indexedDB/ActorsChild.cpp @@ +1058,5 @@ > default: > MOZ_CRASH("Unknown response type!"); > } > > + GetOpenDBRequest()->NoteComplete(); Can this not return nullptr? If not, why isn't it named OpenDBRequest()? ::: dom/workers/WorkerScope.cpp @@ +68,5 @@ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mConsole) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPerformance) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLocation) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNavigator) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIndexedDB) I think this belongs in a different cset.

Comment on attachment 8528830 [details] [diff] [review] Part 8: Fix versionchange transactions on canceled workers, v1 Review of attachment 8528830 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsChild.cpp @@ +1167,5 @@ > IDBVersionChangeEvent::Create(mRequest, > type, > aCurrentVersion, > mRequestedVersion); > + MOZ_ASSERT(blockedEvent); You could put the assertion outside of the if. @@ +1370,5 @@ > + MOZ_ASSERT(!NS_IsMainThread()); > + > + // Report this to the console. > + IDB_REPORT_INTERNAL_ERR(); > + //MOZ_ALWAYS_TRUE(aActor->SendDeleteMe()); Why do you have commented out code in this patch? @@ +1462,5 @@ > IDBVersionChangeEvent::Create(mDatabase, > type, > aOldVersion, > aNewVersion.get_uint64_t()); > + MOZ_ASSERT(versionChangeEvent); outside the switch?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #70) > You could put the assertion outside of the if. But then you can't tell which case failed from the stdout. I like having the different line numbers in the output. > Why do you have commented out code in this patch? Fixed. > outside the switch? Again with being able to tell which case was hit via stdout.

This patch was somehow missing from this bug... Oops.

(In reply to Andrea Marchesini (:baku) from comment #64) > remove mozilla::dom::workers:: Unfortunately I can't because the new class name is also WorkerFeature so I have to distinguish it from the base. > NIT: I'm not a native english speaker, but this is called 'runnable', but > actually it is not a runnable. What about a different name? PreemptingRunnableInfo! > for (uint32_t index = 0; ; index < mPreemptingRunnables.Length();) { > PreemptingRunnable& preemptingRunnable = mPreemptingRunnables[index]; > if (preemptingRunnable.mRecursionDepth == aRecursionDepth) { > preemtingRunnable.mRunnable->Run(); > mPreemptingRunnables.RemoveAt(index); > } else { > ++index; > } > } Unfortunately I have to guard against the runnable mutating the array, so I can't do this. > remove mPreemtingRunnables.Length() == 1. Don't we have to add a dummy event > in any case if we don't have a pending one? No, we are only worried about the specific case tested for here. If the length is > 1 then we must have already dispatched a dummy runnable (assuming that there were no other events in the queue at the time). > Do we have tests that test the ordering of IDB events in the presence of > sync XHR/etc? Not specifically, no, but all IPDL messages are dispatched to the main event queue only so there's no possibility that they could run during one of our sync loops. > Can this not return nullptr? If not, why isn't it named OpenDBRequest()? It can return null after ActorDestroy. I added another assertion here just to make it more clear. > I think this belongs in a different cset. It's now in Part 2.5!

(In reply to Andrea Marchesini (:baku) from comment #65) > can you use nsCancelableRUnnable instead? Yep > then remove this line and NS_DECL_NSIRUNNABLE && > NS_DECL_NSICANCELABLERUNNABLE. Well, I want my own name in the leak stats so I left NS_DECL_ISUPPORTS_INHERITED. Then I have to keep the other two lines because nsCancelableRunnable actually implements Run/Cancel with no-ops (I have no idea why that makes any sense). > you can remove this line. Changed to NS_IMPL_ISUPPORTS_INHERITED0 > In theory mRunnable should exist here because Run() cannot be called after > mRunnable. Well, that's what the worker code does currently... But since nsIRunnable/nsICancelable are so generic I think it's smarter to have code here to handle misuse. > I don't think this is enough to be 100% sure that we can call > MOZ_ASSERT(BackgroundChild::GetForCurrenThread()) > somewhere else. Correct? It's not bulletproof, no, but I don't think it's worth worrying about at the moment. In order for us to lose this race we'd have to have PBackground thread startup be somehow slower than Necko loading the worker's script.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #74) > In order for us to lose this race we'd have to have PBackground > thread startup be somehow slower than Necko loading the worker's script. Fixing that isn't very difficult (could just artificially delay the scriptloader while waiting for PBackground to connect), but I don't think it's worth the effort really.

Actually, I don't want the threadsafe refcount, so I just left CancelableRunnableWrapper pretty much as it was.

Comment on attachment 8533407 [details] [diff] [review] Part 11: Fix the aborted transaction warning for workers, v1.1 Oops, this was actually part 11.

(In reply to Andrea Marchesini (:baku) from comment #67) > return Run(); ? No, nsICancelable specifies what should be returned, and when.

Comment on attachment 8536801 [details] [diff] [review] Part 7: Fix blobs on workers, v1.1 Review of attachment 8536801 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBObjectStore.cpp @@ +612,5 @@ > + if (NS_IsMainThread()) { > + if (aDatabase && aDatabase->GetParentObject()) { > + parent = aDatabase->GetParentObject(); > + } else { > + parent = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx)); nit, extra whitespace after parent

Comment on attachment 8536819 [details] [diff] [review] Part 5: Fix error events fired at global scope, v1 Review of attachment 8536819 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBFactory.cpp @@ +171,5 @@ > factory->mTabChild = TabChild::GetFrom(aWindow); > factory->mInnerWindowID = aWindow->WindowID(); > factory->mPrivateBrowsingMode = > loadContext && loadContext->UsePrivateBrowsing(); > + factory->mUseInnerWindowID = true; This should not be needed. @@ +317,5 @@ > factory->mOwningObject = aOwningObject; > mozilla::HoldJSObjects(factory.get()); > > + if (aOptionalInnerWindowID.WasPassed()) { > + factory->mInnerWindowID = aOptionalInnerWindowID.Value(); This could be written like: factory->mInnerWindowID = aOptionalInnerWindowID.Value(); MOZ_ASSERT(factory->mInnerWindowID); @@ +438,5 @@ > +uint64_t > +IDBFactory::InnerWindowID() const > +{ > + AssertIsOnOwningThread(); > + MOZ_ASSERT(mUseInnerWindowID); you don't need to use mUseInnerWindowID because mInnerWindowID is always > 0. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +36,3 @@ > #include "nsThreadUtils.h" > #include "prlog.h" > +#include "WorkerScope.h" alphabetic order.

Comment on attachment 8528832 [details] [diff] [review] Part 10: Add a sync message for FileReaderSync, v1 Review of attachment 8528832 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/Blob.cpp @@ +239,2 @@ > ConstructFileDescriptorSet(ManagerType* aManager, > + nsTArray<FileDescriptor>& aFDs, Try a move reference here. It's more semantically correct. @@ +276,5 @@ > +} > + > +void > +OptionalFileDescriptorSetToFDs(OptionalFileDescriptorSet& aOptionalSet, > + nsTArray<FileDescriptor>& aFDs) You could return an nsTArray&& too.

Comment on attachment 8536776 [details] [diff] [review] Part 2.5: Add factory methods for workers Review of attachment 8536776 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +91,5 @@ > #include "WorkerScope.h" > > +#ifdef XP_WIN > +#undef PostMessage > +#endif Nooooooooooooooooooooooooooooooooooooo ::: dom/workers/WorkerScope.cpp @@ +321,5 @@ > already_AddRefed<IDBFactory> > WorkerGlobalScope::GetIndexedDB(ErrorResult& aErrorResult) > { > + mWorkerPrivate->AssertIsOnWorkerThread(); > + MOZ_ASSERT(mWorkerPrivate->IsIndexedDBAllowed()); We never check this before asserting it. This needs to return null (here or somewhere else).

Comment on attachment 8528836 [details] [diff] [review] Part 13: Run properly structured mochitests as worker tests, v1 Review of attachment 8528836 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/test/helpers.js @@ +60,5 @@ > + let src = scripts[i].src; > + let match = src.match(/indexedDB\/test\/unit\/(test_[^\/]+\.js)$/); > + if (match && match.length == 2) { > + testScriptPath = src; > + testScriptFilename = match[1]; break? ::: dom/indexedDB/test/unit/test_setVersion_events.js @@ +2,5 @@ > * Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ > */ > > +let disableWorkerTest = "This test uses SpecialPowers"; This test probably can and should be fixed.

https://hg.mozilla.org/mozilla-central/rev/9d0ed89e7c58 https://hg.mozilla.org/mozilla-central/rev/8d40b95ffdbf

We should blog about this on hacks once it has moved down a train or two.

Release Note Request (optional, but appreciated) [Why is this notable]: Needed by people working heavily with IndexedDB [Suggested wording]: Worker threads have access to IndexedDB [Links (documentation, blog post, etc)]: See comment 90 Sebastian

relnoted as "IndexDB now accessible from worker threads"

Please make sure that you spell the name of the feature correctly ;)

Oh. That's a great typo. Thanks for catching it!

Covered in https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers