Bug 798875 (SyncIDB)

Implement IndexedDB synchronous API

RESOLVED WONTFIX

Status

()

P3
normal
RESOLVED WONTFIX
6 years ago
20 days ago

People

(Reporter: janv, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [games:p?][Async:ready])

Attachments

(2 attachments, 9 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

6 years ago
The implementation will be done as a diploma thesis, here's the goal of the thesis:

The goal of this work is to implement the IndexedDB synchronous API in Firefox browser and in the new mobile platform Firefox OS. These platforms already support the asynchronous API of the database, but the missing implementation of the synchronous API, which is currently unsupported in the other known web browsers too, partially prevents broader deployment and increase of popularity of IndexedDB among developers of web applications.

1. Describe HTML5 applications in general and local storage of data (especially IndexedDB database)

2. Implementation of the synchronous IndexedDB API in Firefox / Firefox OS

3. Describe the implementation in terms of architecture, description of objects, interactions between them as well as description of communication between threads.

4. Evaluation of results
(Reporter)

Updated

6 years ago
Assignee: nobody → swenruzicka
(Reporter)

Comment 2

6 years ago
Created attachment 675170 [details] [diff] [review]
webidl template
(Reporter)

Updated

6 years ago
Depends on: 803106
(Reporter)

Updated

6 years ago
Depends on: 806184
(Reporter)

Updated

6 years ago
Depends on: 809888
(Reporter)

Updated

6 years ago
Depends on: 812783
Created attachment 682874 [details] [diff] [review]
Initial WebIDL

New attachment adds WebIDL files to dom/webidl and *.h *.cpp files to dom/workers with stub methods for sync API of IDB.

DOMStringList attributes and parameters are commented out because they are not yet available (see bug 803106 ) and the same with callbacks (see bug 812783 ).
(Reporter)

Updated

6 years ago
Depends on: 820339
(Reporter)

Updated

6 years ago
Depends on: 832883
(Reporter)

Comment 4

6 years ago
Created attachment 705318 [details] [diff] [review]
Part 1: Basic infrastructure
Attachment #705318 - Flags: review?(bent.mozilla)
(Reporter)

Updated

6 years ago
Depends on: 834141
(Reporter)

Updated

6 years ago
Depends on: 816088
Created attachment 708944 [details] [diff] [review]
wip

The actual state of work.
In the case of testing this patch need to be applied on top of the ipcthreadbase patch from bug 809888

Methods(m)/properties(p) which mostly work:

IDBDatabaseSync
 p: name,version,objectStoreNames
 m: transaction

IDBFactorySync
 m: open, cmp
 
IDBTransactionSync
 p: mode, db,
 m: objectStore

IDBObjectStoreSync
 p: transaction,
 m: get, delete, clear, index, count

IDBIndexSync
 p: objectStore
 m: openKeyCursor, get, getKey, count
 
IDBCursorSync
 p: source, direction, key, primaryKey
 m: continue
(Reporter)

Comment 6

6 years ago
Vendo is going to submit a new patch soon. The implementation is almost feature complete.

Stuff that needs to be implemented:
IDBFactory::DeleteDatabase()
IDBTransactionSync::Error()
IDBTransactionSync::Abort()
onversionchange handling
Created attachment 733426 [details] [diff] [review]
IDBsync

For more information about current state of implementation visit:
https://etherpad.mozilla.org/sync-idb
Attachment #675170 - Attachment is obsolete: true
Attachment #682874 - Attachment is obsolete: true
Attachment #705318 - Attachment is obsolete: true
Attachment #708944 - Attachment is obsolete: true
Attachment #705318 - Flags: review?(bent.mozilla)
Created attachment 746580 [details] [diff] [review]
Sync IDB API (diploma thesis version)

This version of the synchronous IDB API patch was officially attached to the diploma thesis (final version for university).
Attachment #733426 - Attachment is obsolete: true
Hi, vendo.
It would be great to have this feature. Could you please divide the patch into smaller chunks, so as to simplify the work of reviewers?
Flags: needinfo?(vendo.ruzicka)
(Reporter)

Comment 10

5 years ago
I just finished work on bringing the sync IDB patch up to date and I also rebased it on top of the temp storage, bug 785884.

Sync IDB is a Q3 goal and we will be talking about it during the work week in Toronto.
I've started working on an initial review and Ben Turner will do a super review after the first one is addressed.

We will split the patch into two parts at least, the implementation and the tests. There's actually a standalone patch already, bug 809888
Flags: needinfo?(vendo.ruzicka)
(Reporter)

Comment 11

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until July 1st> from comment #9)
> Hi, vendo.
> It would be great to have this feature. Could you please divide the patch
> into smaller chunks, so as to simplify the work of reviewers?

I forgot to ask, what is your use case of sync IDB ?
> I just finished work on bringing the sync IDB patch up to date and I also rebased it on top of the temp storage, bug 785884.

Ah, great. I was a bit worried because I saw a huge patch without a review flag, so I was afraid it would stay here and bitrot.

> I forgot to ask, what is your use case of sync IDB ?

I have no immediate use case, but I'm tech leading the Async project [1] and I'd like to evaluate sync IDB to determine whether I should suggest this as a storage solution for add-on authors.

[1] https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed&namedcmd=Async
(Reporter)

Updated

5 years ago
No longer depends on: 832883
(Reporter)

Comment 13

5 years ago
I just found this:
"Rationale:  Workers are coming along great, but of course it would be great to have more: IndexedDB access, mozTCPSocket access, better debug and profile support for them.
Rationale for Gaia:  Email app uses a worker with moxTCPSocket and IndexedDB. Right now it proxies back to the main thread to do IO with those capabilities. It complicates the code. It is also difficult to debug issues in the workers as well as get accurate info on profiling what happens in them."

The current implementation of sync IDB API is single process only, we will need to add support for child processes too.
(In reply to Jan Varga [:janv] from comment #13)
> Rationale for Gaia:  Email app uses a worker with moxTCPSocket and
...
> The current implementation of sync IDB API is single process only, we will
> need to add support for child processes too.

For Gaia, I think it's always expected that each app (with its own origin) will only ever operate within a single process, so that might eliminate that concern.

For the Gaia e-mail app (in regards to that blurb), we'd really prefer the async API because of our architecture.  But we can use the sync API in a sub-worker so that we're remoting to another background thread rather than the main thread.
(Reporter)

Comment 15

5 years ago
(In reply to Andrew Sutherland (:asuth) from comment #14)
> (In reply to Jan Varga [:janv] from comment #13)
> > Rationale for Gaia:  Email app uses a worker with moxTCPSocket and
> ...
> > The current implementation of sync IDB API is single process only, we will
> > need to add support for child processes too.
> 
> For Gaia, I think it's always expected that each app (with its own origin)
> will only ever operate within a single process, so that might eliminate that
> concern.

I actually meant that the current implementation works in Firefox Desktop only (one single process) and that will need to add support for workers created from child processes.

> 
> For the Gaia e-mail app (in regards to that blurb), we'd really prefer the
> async API because of our architecture.  But we can use the sync API in a
> sub-worker so that we're remoting to another background thread rather than
> the main thread.

ok, thanks for this info

Comment 16

5 years ago
(In reply to Andrew Sutherland (:asuth) from comment #14)
> For the Gaia e-mail app (in regards to that blurb), we'd really prefer the
> async API because of our architecture.  But we can use the sync API in a
> sub-worker so that we're remoting to another background thread rather than
> the main thread.
This wording worries me a bit. "But we can use the sync API in a sub-worker" suggests that you could also decide otherwise (that is using the sync API in the main thread). Last I discussed this with Jonas Sicking [1] on this topic, it was clear that this cannot be an option.

With the current implementation (or at least intent on this bug), will it be possible to use the sync API on the main thread?

[1] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0648.html and following messages
(In reply to David Bruant from comment #16)
> This wording worries me a bit. "But we can use the sync API in a sub-worker"
> suggests that you could also decide otherwise (that is using the sync API in
> the main thread). Last I discussed this with Jonas Sicking [1] on this
> topic, it was clear that this cannot be an option.

It is indeed my understanding that the sync API would never be exposed to the main thread under any circumstances.

What I was trying to express is that for the e-mail app, what we really want is async access to IndexedDB on a worker thread.  We currently use only one worker and having it block on I/O would be very bad.  But it seems like we could cobble together an async IndexedDB impementation out of a sub-worker and a synchronous-on-workers-only IndexedDB implementation if that is all we have.  Currently, we see a non-trivial amount of main-thread jank (on a single-core non-beefy Firefox OS device) just from the structured clone operations bouncing our calls to the async IndexedDB off the main thread.
(Reporter)

Comment 18

5 years ago
(In reply to David Bruant from comment #16)
> With the current implementation (or at least intent on this bug), will it be
> possible to use the sync API on the main thread?

of course not :)

using the sync API in a subworker should be ok
Whiteboard: [games:p1]
(Reporter)

Comment 19

5 years ago
Created attachment 792358 [details] [diff] [review]
A rollup patch

A rollup patch for those who want to try it with the current m-c.

https://tbpl.mozilla.org/?tree=Try&rev=b0246a1406d2
(Reporter)

Comment 20

5 years ago
Created attachment 792717 [details] [diff] [review]
A rollup patch

This one actually builds on windows:
https://tbpl.mozilla.org/?tree=Try&rev=a932c801c1d8
Attachment #792358 - Attachment is obsolete: true
(Reporter)

Comment 21

5 years ago
Additional development like review fixes, support for stored files, access from workers in child processes, etc. will be done on a disposable project branch.
https://tbpl.mozilla.org/?tree=Jamun
(Reporter)

Comment 22

5 years ago
Comment on attachment 746580 [details] [diff] [review]
Sync IDB API (diploma thesis version)

Review of attachment 746580 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/DatabaseInfoSync.h
@@ +12,5 @@
> +#include "mozilla/dom/indexedDB/DatabaseInfo.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +struct DatabaseInfoSync : public indexedDB::DatabaseInfoBase

This should be called DatabaseInfoMT and the hashtable should be protected by a mutex.
It would be also nice to have a test that creates multiple workers which access the same database.
(Reporter)

Comment 23

5 years ago
Comment on attachment 746580 [details] [diff] [review]
Sync IDB API (diploma thesis version)

Review of attachment 746580 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/IDBCursorSync.webidl
@@ +6,5 @@
> + * The origin of this IDL file is http://www.w3.org/TR/IndexedDB/
> + */
> +
> +interface IDBCursorSync {
> +  readonly attribute object source;

We decided to follow the style used in the specification [1], bug 888598.
Please adjust all the webidl files to match the spec.

[1] http://www.w3.org/TR/2012/WD-IndexedDB-20120524/

::: dom/webidl/IDBDatabaseSync.webidl
@@ +36,5 @@
> +              optional unsigned long timeout);
> +
> +  [Throws]
> +  /* FileHandleSync */ any
> +  mozCreateFileHandle(DOMString name,

This is a mozilla specific method, thus it should be move to "partial interface IDBDatabaseSync" (see how it is done elsewhere).

Also, you can remove the "moz" prefix in the implementation, take a look at 'binaryNames' for IDBIndex in Bindings.conf
I think you can do that for IDBDatabase interface too.

.mozStorage in IDBDatabaseSync should be moved to a partial interface too and make it pref controlled please
(Reporter)

Comment 24

5 years ago
Comment on attachment 792717 [details] [diff] [review]
A rollup patch

Review of attachment 792717 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3397,5 @@
> +                # We have a specialization of Optional that will use a
> +                # Rooted for the storage here.
> +                declType = CGGeneric("JS::Handle<JSObject*>")
> +            else:
> +                declType = CGGeneric("JS::Rooted<JSObject*>")

Vendo, please file a new bug for this and cc bz. This is actually a followup for bug 868715 part 10, code for callbacks in workers hasn't been updated properly I think.
Whiteboard: [games:p1] → [games:p1][async]
(Reporter)

Updated

5 years ago
Depends on: 917182
(Reporter)

Updated

5 years ago
Attachment #746580 - Attachment description: Sync IDB API → Sync IDB API (diploma thesis version)
(Reporter)

Updated

5 years ago
Depends on: 919268
(Reporter)

Comment 25

5 years ago
Created attachment 808274 [details] [diff] [review]
diff between m-c and jamun
Attachment #792717 - Attachment is obsolete: true
(Reporter)

Comment 26

5 years ago
Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

Review of attachment 808274 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/DatabaseInfoMT.h
@@ +29,5 @@
> +  already_AddRefed<DatabaseInfoMT> Clone();
> +
> +  nsCString id;
> +
> +  static StaticMutex sDatabaseInfoMutex;

Vendo, the mutex only protects Get/Put/Remove at the moment, we have to protect other methods too.
For example GetObjectStore(), take a look at this crash:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28204915&tree=Jamun&full=1#error1
(Reporter)

Comment 27

5 years ago
Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

Review of attachment 808274 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/IDBDatabaseSync.cpp
@@ +524,5 @@
> +
> +  if (mUpgradeNeeded) {
> +    if (aUpgradeCallback) {
> +      ErrorResult rv;
> +      aUpgradeCallback->Call(mFactory, *mTransaction, 99, rv);

Vendo, we need to pass a real number here and it seems we don't have a test for it.
Async test suite has it in test_setVersion_events.js
(Reporter)

Comment 28

5 years ago
Vendo, another thing ... please convert all NS_ASSERTION that we added to MOZ_ASSERT
(Reporter)

Comment 29

5 years ago
It would be also nice to have getAllKeys and openKeyCursor in IDBObjectStoreSync. These were added to async API in bug 920633 and bug 920800.
(Reporter)

Comment 30

5 years ago
IDBKeyRange is now shared by the main thread and workers
(Reporter)

Comment 31

5 years ago
Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

Review of attachment 808274 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerScope.cpp
@@ +1180,3 @@
>        !EventBinding::GetConstructorObject(aCx, global) ||
>        !FileReaderSyncBinding_workers::GetConstructorObject(aCx, global) ||
> +      !IDBCursorSyncBinding_workers::GetProtoObject(aCx, global) ||

Vendo, all these GetProtoObject() should be changed to GetConstructorObject()

::: dom/workers/test/indexedDB/objectCursors_worker.js
@@ +57,5 @@
> +
> +    }
> +  }
> +
> +  ok(true, "Test successfully completed");

This can be: info("Test successfully completed"), you will need to add info() the helpers.
Most of the reviewers comments has been already addressed on jamun.
I had a wonderful time at the Brussels summit and fixed comment#31 only today.
OpenKeyCursor function will be added soon to IDBObjectStoreSync.
OpenKeyCursor function has been added.
(Reporter)

Updated

5 years ago
Depends on: 928312, 925531, 919885, 923054

Updated

5 years ago
Blocks: 710398
(Reporter)

Comment 34

5 years ago
Created attachment 8336859 [details] [diff] [review]
the current diff between m-c and jamun
Attachment #808274 - Attachment is obsolete: true
Attachment #8336859 - Flags: feedback?(bent.mozilla)
(Reporter)

Comment 35

5 years ago
Just to bring some context, we're currently pondering the possibility to quickly rewrite the sync IDB impl using Ben's new event loop and direct IPC in workers once it's ready or just let Ben review the current patch.

Vendo said he's available to help with the rewrite.
Whiteboard: [games:p1][async] → [games:p1][Async:ready]
(Reporter)

Updated

5 years ago
Depends on: 931887
(Reporter)

Comment 36

5 years ago
Created attachment 8366330 [details] [diff] [review]
rebased (now depends on patches in bug  923054 and bug 803106)
Attachment #8336859 - Attachment is obsolete: true
Attachment #8336859 - Flags: feedback?(bent.mozilla)
(Reporter)

Comment 37

5 years ago
Update:
The current plan is to land existing patch, pref it off, get feedback, enable it once some other vendor is willing to implement it
Whiteboard: [games:p1][Async:ready] → [games:p?][Async:ready]
Keywords: dev-doc-needed

Updated

9 months ago
Priority: -- → P3

Updated

28 days ago
Assignee: vendo.ruzicka → nobody
The synchronous API didn't happen and is definitely no longer going to happen.
Status: NEW → RESOLVED
Last Resolved: 26 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.