Last Comment Bug 701634 - (AsyncIDB) IndexedDB: Support database access from worker threads
(AsyncIDB)
: IndexedDB: Support database access from worker threads
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal with 22 votes (vote)
: mozilla37
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
: 1032094 (view as bug list)
Depends on: 487070 idbwebidl 827823 845545 853893 PBackground IndexedDB-on-PBackground 1054638 1112716 1152174
Blocks: 1040611 IndexedDB 898706 916196 apis-in-workers 936302 940273 1032096 ServiceWorkers-v1 1083089 1113340 1161063
  Show dependency treegraph
 
Reported: 2011-11-10 23:20 PST by Christopher Blizzard (:blizzard)
Modified: 2015-05-04 08:09 PDT (History)
78 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
37+


Attachments
patch 1 - QuotaManager helper class in workers (16.72 KB, patch)
2013-11-25 08:58 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 - IDB - WIP (147.49 KB, patch)
2013-12-02 10:00 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 1 - TransactionThreadPool without DOM elements. (36.81 KB, patch)
2013-12-10 10:27 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 1 - TransactionThreadPool without DOM elements. (36.66 KB, patch)
2013-12-12 07:23 PST, Andrea Marchesini (:baku)
bent.mozilla: review+
Details | Diff | Review
transaction.patch (36.69 KB, patch)
2013-12-19 08:13 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
Part 1: WebIDL stubs and pref, v1 (24.57 KB, patch)
2014-11-25 21:55 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review+
Details | Diff | Review
Part 2: Refactor 3rd party checks, v1 (18.63 KB, patch)
2014-11-25 21:59 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review+
Details | Diff | Review
Part 3: Fix open() for workers, v1 (9.37 KB, patch)
2014-11-25 22:03 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 4: Fix wrapping of binding objects, v1 (6.32 KB, patch)
2014-11-25 22:06 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review+
Details | Diff | Review
Part 5: Fix error events fired at global scope, v1 (13.48 KB, patch)
2014-11-25 22:08 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review-
Details | Diff | Review
Part 6: Hook transaction lifetime to the worker event loop, v1 (23.64 KB, patch)
2014-11-25 22:15 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
amarchesini: feedback+
Details | Diff | Review
Part 7: Fix blobs on workers, v1 (28.33 KB, patch)
2014-11-25 22:17 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Part 8: Fix versionchange transactions on canceled workers, v1 (18.11 KB, patch)
2014-11-25 22:18 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review-
amarchesini: feedback+
Details | Diff | Review
Part 9: Un-inline the RemoteInputStream (no functional changes), v1 (16.16 KB, patch)
2014-11-25 22:19 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 10: Add a sync message for FileReaderSync, v1 (30.76 KB, patch)
2014-11-25 22:23 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 11: Fix the aborted transaction warning for workers, v1 (9.59 KB, patch)
2014-11-25 22:24 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: feedback-
Details | Diff | Review
Part 12: Fix small issues revealed by tests, v1 (4.38 KB, patch)
2014-11-25 22:27 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
amarchesini: feedback+
Details | Diff | Review
Part 13: Run properly structured mochitests as worker tests, v1 (20.12 KB, patch)
2014-11-25 22:28 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 1: WebIDL stubs and pref, v1.1 (26.52 KB, patch)
2014-12-08 11:52 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
amarchesini: feedback+
Details | Diff | Review
Part 2: Refactor 3rd party checks, v1.1 (18.60 KB, patch)
2014-12-08 12:07 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
amarchesini: feedback+
Details | Diff | Review
Part 4: Fix wrapping of binding objects, v1.1 (6.06 KB, patch)
2014-12-08 12:11 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Review
Part 5: Fix error events fired at global scope, v1.1 (20.29 KB, patch)
2014-12-08 13:51 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Part 11: Fix the aborted transaction warning for workers, v1.1 (11.18 KB, patch)
2014-12-08 15:45 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review+
Details | Diff | Review
Part 11: Fix the aborted transaction warning for workers, v1.1 (11.18 KB, patch)
2014-12-15 09:40 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Part 8: Fix versionchange transactions on canceled workers, v1.1 (17.85 KB, patch)
2014-12-15 12:53 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
bent.mozilla: feedback+
Details | Diff | Review
Part 2.5: Add factory methods for workers (23.83 KB, patch)
2014-12-15 13:31 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 6: Hook transaction lifetime to the worker event loop, v1.1 (22.45 KB, patch)
2014-12-15 13:41 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
bent.mozilla: feedback+
Details | Diff | Review
Part 7: Fix blobs on workers, v1.1 (28.09 KB, patch)
2014-12-15 13:59 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Part 7: Fix blobs on workers, v1.1 (28.08 KB, patch)
2014-12-15 14:08 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Part 5: Fix error events fired at global scope, v1 (20.26 KB, patch)
2014-12-15 14:30 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
amarchesini: review+
Details | Diff | Review
Part 14: Loose ends (28.27 KB, patch)
2014-12-16 13:47 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Part 14: Loose ends (4.64 KB, patch)
2014-12-16 13:48 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review

Description Christopher Blizzard (:blizzard) 2011-11-10 23:20:45 PST
There's a way to access an IndexedDB from a worker thread with a synchronous API.  We should support it.
Comment 1 Jonas Sicking (:sicking) 2011-11-10 23:23:57 PST
Actually, per spec, workers are supposed to have access to both the sync and the async API.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-11 05:15:57 PST
I note that in the current worker setup doing this will be *excruciatingly* painful.
Comment 3 Christopher Blizzard (:blizzard) 2011-11-11 08:02:55 PST
Why so painful?  Please tell us more.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-11 08:03:39 PST
There is no XPConnect in workers, so all of the bindings must be written manually using JSAPI.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-11 08:05:32 PST
(The long term plan here is to have tools that automatically generate that goop, but those don't exist yet.)
Comment 6 Olli Pettay [:smaug] 2011-11-11 08:52:24 PST
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.
Comment 7 Jonas Sicking (:sicking) 2011-11-11 10:36:54 PST
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.
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-21 22:13:42 PST
Also, let's remove from tracker bug 687337. That one is about getting our main thread async API up to spec.
Comment 9 dgrogan(chrome) 2012-08-07 18:01:41 PDT
Are there separate bugs for the async and sync api being available from workers?
Comment 10 Andrew Overholt [:overholt] 2012-08-14 09:45:32 PDT
Andrea, do we have bugs for the sync API?
Comment 11 Andrea Marchesini (:baku) 2012-08-14 09:52:01 PDT
I don't think so. Not yet.
Comment 12 Andrea Marchesini (:baku) 2012-08-16 00:44:08 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=783190
Comment 13 Deni 2012-09-19 11:53:19 PDT
Are there plans for implementing Async or Sync IndexedDB api in web workers in FF soon?
Comment 14 Andrew Overholt [:overholt] 2012-09-25 05:54:20 PDT
(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.
Comment 15 Jan Varga [:janv] 2012-10-07 02:19:41 PDT
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).
Comment 16 Ben Vanik 2012-12-29 22:13:58 PST
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.
Comment 17 max 2013-07-23 15:36:57 PDT
+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)
Comment 18 Olli Pettay [:smaug] 2013-07-23 16:16:59 PDT
(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.
Comment 19 Jan Varga [:janv] 2013-10-01 23:21:48 PDT
Andrea, why did you make this bug depend on bug 783190 ?
Comment 20 Zach Lym 2013-11-11 18:24:35 PST
(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?  :)
Comment 21 Andrea Marchesini (:baku) 2013-11-25 08:58:57 PST
Created attachment 8337810 [details] [diff] [review]
patch 1 - QuotaManager helper class in workers

This is a helper class that gives access to QuotaManager mainthread only methods from workers.
Comment 22 Andrea Marchesini (:baku) 2013-12-02 10:00:12 PST
Created attachment 8341165 [details] [diff] [review]
patch 2 - IDB - WIP

Still needs to have bug 914762 landed.
Comment 23 Andrea Marchesini (:baku) 2013-12-10 10:27:53 PST
Created attachment 8345387 [details] [diff] [review]
patch 1 - TransactionThreadPool without DOM elements.
Comment 24 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-12-11 16:45:06 PST
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?
Comment 25 Andrea Marchesini (:baku) 2013-12-12 07:22:54 PST
> > +
> > +    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?
Comment 26 Andrea Marchesini (:baku) 2013-12-12 07:23:46 PST
Created attachment 8346577 [details] [diff] [review]
patch 1 - TransactionThreadPool without DOM elements.
Comment 27 Jonas Sicking (:sicking) 2013-12-12 14:54:44 PST
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 28 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-12-19 06:49:21 PST
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.
Comment 29 Andrea Marchesini (:baku) 2013-12-19 08:13:06 PST
Created attachment 8350102 [details] [diff] [review]
transaction.patch

rebased
Comment 30 Jan Varga [:janv] 2014-03-18 11:06:00 PDT
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.
Comment 31 Markus Siebeneicher 2014-04-05 11:42:29 PDT
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!
Comment 32 nanchet 2014-06-08 15:13:01 PDT
(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!
Comment 33 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-06-08 15:57:05 PDT
This has sorta become a meta bug. Development on this project is happening in the jamun project branch (http://hg.mozilla.org/projects/jamun).
Comment 34 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-06-30 06:06:52 PDT
*** Bug 1032094 has been marked as a duplicate of this bug. ***
Comment 35 Paul Rouget [:paul] 2014-07-02 01:21:31 PDT
What's the status of this bug? Is it actively being worked on? Anything I can do to help?
Comment 36 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-07-02 10:02:42 PDT
Yep, we're on it, it's top priority for several of us.
Comment 37 Andrea Marchesini (:baku) 2014-11-17 04:31:11 PST
I guess bent is working on this.
Comment 38 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 21:55:53 PST
Created attachment 8528820 [details] [diff] [review]
Part 1: WebIDL stubs and pref, v1
Comment 39 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 21:59:52 PST
Created attachment 8528822 [details] [diff] [review]
Part 2: Refactor 3rd party checks, v1
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:03:24 PST
Created attachment 8528824 [details] [diff] [review]
Part 3: Fix open() for workers, v1
Comment 41 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:06:41 PST
Created attachment 8528826 [details] [diff] [review]
Part 4: Fix wrapping of binding objects, v1
Comment 42 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:08:53 PST
Created attachment 8528827 [details] [diff] [review]
Part 5: Fix error events fired at global scope, v1
Comment 43 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:15:06 PST
Created attachment 8528828 [details] [diff] [review]
Part 6: Hook transaction lifetime to the worker event loop, v1
Comment 44 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:17:01 PST
Created attachment 8528829 [details] [diff] [review]
Part 7: Fix blobs on workers, v1
Comment 45 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:18:00 PST
Created attachment 8528830 [details] [diff] [review]
Part 8: Fix versionchange transactions on canceled workers, v1
Comment 46 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:19:39 PST
Created attachment 8528831 [details] [diff] [review]
Part 9: Un-inline the RemoteInputStream (no functional changes), v1

(This is just moving code, no functional changes)
Comment 47 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:23:07 PST
Created attachment 8528832 [details] [diff] [review]
Part 10: Add a sync message for FileReaderSync, v1
Comment 48 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:24:26 PST
Created attachment 8528833 [details] [diff] [review]
Part 11: Fix the aborted transaction warning for workers, v1
Comment 49 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:27:42 PST
Created attachment 8528835 [details] [diff] [review]
Part 12: Fix small issues revealed by tests, v1
Comment 50 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:28:47 PST
Created attachment 8528836 [details] [diff] [review]
Part 13: Run properly structured mochitests as worker tests, v1
Comment 51 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-25 22:30:26 PST
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.
Comment 52 James Lal [:lightsofapollo] (inactive) 2014-11-25 22:32:05 PST
\o/
Comment 53 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-26 07:43:49 PST
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?
Comment 54 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-11-26 07:50:32 PST
Kyle, feel free to bounce reviews to anyone else you think could do them.
Comment 55 Andrea Marchesini (:baku) 2014-11-26 08:42:20 PST
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 56 Andrea Marchesini (:baku) 2014-12-04 11:22:07 PST
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 57 Andrea Marchesini (:baku) 2014-12-05 11:05:48 PST
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 58 Andrea Marchesini (:baku) 2014-12-05 15:47:27 PST
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.
Comment 59 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-08 11:52:20 PST
Created attachment 8533287 [details] [diff] [review]
Part 1: WebIDL stubs and pref, v1.1

(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?
Comment 60 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-08 12:07:28 PST
Created attachment 8533294 [details] [diff] [review]
Part 2: Refactor 3rd party checks, v1.1

(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.
Comment 61 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-08 12:11:35 PST
Created attachment 8533295 [details] [diff] [review]
Part 4: Fix wrapping of binding objects, v1.1
Comment 62 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-08 13:51:37 PST
Created attachment 8533358 [details] [diff] [review]
Part 5: Fix error events fired at global scope, v1.1

(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 63 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-08 15:45:37 PST
Created attachment 8533407 [details] [diff] [review]
Part 11: Fix the aborted transaction warning for workers, v1.1
Comment 64 Andrea Marchesini (:baku) 2014-12-09 10:08:59 PST
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 65 Andrea Marchesini (:baku) 2014-12-09 10:45:30 PST
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 66 Andrea Marchesini (:baku) 2014-12-15 09:35:11 PST
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 67 Andrea Marchesini (:baku) 2014-12-15 09:36:36 PST
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(); ?
Comment 68 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 09:40:59 PST
Created attachment 8536664 [details] [diff] [review]
Part 11: Fix the aborted transaction warning for workers, v1.1

Rebased part 11 over changes from earlier review notes.
Comment 69 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-15 11:17:47 PST
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 70 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-15 11:45:52 PST
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?
Comment 71 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 12:53:58 PST
Created attachment 8536756 [details] [diff] [review]
Part 8: Fix versionchange transactions on canceled workers, v1.1

(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.
Comment 72 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 13:31:23 PST
Created attachment 8536776 [details] [diff] [review]
Part 2.5: Add factory methods for workers

This patch was somehow missing from this bug... Oops.
Comment 73 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 13:41:11 PST
Created attachment 8536784 [details] [diff] [review]
Part 6: Hook transaction lifetime to the worker event loop, v1.1

(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!
Comment 74 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 13:59:04 PST
Created attachment 8536793 [details] [diff] [review]
Part 7: Fix blobs on workers, v1.1

(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.
Comment 75 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 14:00:55 PST
(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.
Comment 76 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 14:08:24 PST
Created attachment 8536801 [details] [diff] [review]
Part 7: Fix blobs on workers, v1.1

Actually, I don't want the threadsafe refcount, so I just left CancelableRunnableWrapper pretty much as it was.
Comment 77 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 14:29:52 PST
Comment on attachment 8533407 [details] [diff] [review]
Part 11: Fix the aborted transaction warning for workers, v1.1

Oops, this was actually part 11.
Comment 78 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 14:30:56 PST
Created attachment 8536819 [details] [diff] [review]
Part 5: Fix error events fired at global scope, v1
Comment 79 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-15 16:16:54 PST
(In reply to Andrea Marchesini (:baku) from comment #67)
> return Run(); ?

No, nsICancelable specifies what should be returned, and when.
Comment 80 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-15 16:45:42 PST
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 81 Andrea Marchesini (:baku) 2014-12-16 05:05:27 PST
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 82 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-16 11:28:32 PST
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 83 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-16 11:45:10 PST
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 84 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-16 11:55:05 PST
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.
Comment 85 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-16 13:47:46 PST
Created attachment 8537433 [details] [diff] [review]
Part 14: Loose ends
Comment 86 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-16 13:48:40 PST
Created attachment 8537434 [details] [diff] [review]
Part 14: Loose ends
Comment 87 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-17 07:11:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0ed89e7c58
Comment 88 Ben Turner (not reading bugmail, use the needinfo flag!) 2014-12-17 09:04:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d40b95ffdbf
Comment 90 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-12-17 18:42:09 PST
We should blog about this on hacks once it has moved down a train or two.
Comment 91 Sebastian Zartner [:sebo] 2014-12-17 22:48:18 PST
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
Comment 92 Alexandre MOTTET 2014-12-17 23:19:55 PST Comment hidden (me-too)
Comment 93 Lawrence Mandel [:lmandel] (use needinfo) 2015-01-15 07:17:40 PST
relnoted as "IndexDB now accessible from worker threads"
Comment 94 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2015-01-15 07:22:22 PST
Please make sure that you spell the name of the feature correctly ;)
Comment 95 Lawrence Mandel [:lmandel] (use needinfo) 2015-01-15 07:24:11 PST
Oh. That's a great typo. Thanks for catching it!
Comment 96 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-01-29 10:42:13 PST
Covered in https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers

Note You need to log in before you can comment on or make changes to this bug.