Christopher Blizzard (:blizzard) Reporter
	Description • 13 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 1 • 13 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 13 years ago

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

	Christopher Blizzard (:blizzard) Reporter
	Comment 3 • 13 years ago

Why so painful?  Please tell us more.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 13 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 5 • 13 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 13 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 13 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 8 • 13 years ago

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

	dgrogan (chrome)
	Comment 9 • 12 years ago

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

	Andrew Overholt [:overholt]
	Comment 10 • 12 years ago

Andrea, do we have bugs for the sync API?

	Andrea Marchesini [:baku]
	Comment 11 • 12 years ago

I don't think so. Not yet.

	Andrea Marchesini [:baku]
	Comment 12 • 12 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=783190

	Deni
	Comment 13 • 12 years ago

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

	Andrew Overholt [:overholt]
	Comment 14 • 12 years ago

(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.

	Jan Varga [:janv]
	Comment 15 • 12 years ago

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).

	Ben Vanik
	Comment 16 • 12 years ago

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.

	max
	Comment 17 • 11 years ago

+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)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 11 years ago

(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.

	Jan Varga [:janv]
	Comment 19 • 11 years ago

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

	Zach Lym
	Comment 20 • 11 years ago

(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?  :)

	Andrea Marchesini [:baku]
	Comment 21 • 11 years ago

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

	Andrea Marchesini [:baku]
	Comment 22 • 11 years ago

Still needs to have bug 914762 landed.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 24 • 11 years ago

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?

	Andrea Marchesini [:baku]
	Comment 25 • 11 years ago

> > +
> > +    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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 11 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 28 • 11 years ago

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.

	Andrea Marchesini [:baku]
	Comment 29 • 11 years ago

rebased

	Jan Varga [:janv]
	Comment 30 • 10 years ago

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.

	Markus Siebeneicher
	Comment 31 • 10 years ago

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!

	nanchet
	Comment 32 • 10 years ago

(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!

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 33 • 10 years ago

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

	Paul Rouget [:paul]
	Comment 35 • 10 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 36 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 37 • 10 years ago

I guess bent is working on this.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 46 • 10 years ago

(This is just moving code, no functional changes)

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 51 • 10 years ago

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.

	James Lal [:lightsofapollo]
	Comment 52 • 10 years ago

\o/

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 53 • 10 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 54 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 55 • 10 years ago

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.

	Andrea Marchesini [:baku]
	Comment 56 • 10 years ago

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) ?

	Andrea Marchesini [:baku]
	Comment 57 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 58 • 10 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 59 • 10 years ago

(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?

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 60 • 10 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 62 • 10 years ago

(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.

	Andrea Marchesini [:baku]
	Comment 64 • 10 years ago

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?

	Andrea Marchesini [:baku]
	Comment 65 • 10 years ago

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?

	Andrea Marchesini [:baku]
	Comment 66 • 10 years ago

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?

	Andrea Marchesini [:baku]
	Comment 67 • 10 years ago

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(); ?

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 68 • 10 years ago

Rebased part 11 over changes from earlier review notes.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 69 • 10 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 70 • 10 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 71 • 10 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 72 • 10 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 73 • 10 years ago

(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!

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 74 • 10 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 75 • 10 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 76 • 10 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 77 • 10 years ago

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

Oops, this was actually part 11.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 79 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #67)
> return Run(); ?

No, nsICancelable specifies what should be returned, and when.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 80 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 81 • 10 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 82 • 10 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 83 • 10 years ago

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).

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 84 • 10 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 87 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0ed89e7c58

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 88 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d40b95ffdbf

	Ryan VanderMeulen [:RyanVM]
	Comment 89 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 90 • 10 years ago

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

	Sebastian Zartner [:sebo]
	Comment 91 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 93 • 9 years ago

relnoted as "IndexDB now accessible from worker threads"

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 94 • 9 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 95 • 9 years ago

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

	Chris Mills [:cmills]
	Comment 96 • 9 years ago

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