809888 - Create IPC thread for workers

Status: RESOLVED WONTFIX

(Core :: DOM: Workers, defect)

Comment 1

[Jan Varga [:janv]]

Attachment: initial infrastructure

Comment 2

[Jan Varga [:janv]]

We would like to reuse the existing IndexedDBParent implementation for sync IDB in workers. However, the event loop in workers can't handle IPC messages. A special shared IPC thread should be easier to implement, since it doesn't require fundamental changes in workers implementation.

Comment 3

[Jan Varga [:janv]]

Attachment: initial design

Comment 4

[Jan Varga [:janv]]

Attachment: test.html

Comment 5

[Jan Varga [:janv]]

Attachment: worker.js

Comment 6

[Jan Varga [:janv]]

Ok, so it seems that IPDL just works in this case too.

With the last patch, we are able to run this JS in workers:
var factory = new IDBFactorySync();
var db = factory.open("foo");
db.transaction("test", function(trans) {
  var store = trans.objectStore("test");
  var name = store.get(1);
});

Anyway, there's no handling of JS objects rooting in the patch yet.

Ben, we need your feedback here, if the implementation design is ok then Vendo can start implementing other sync IDB API methods and objects.

Comment 7

[Jan Varga [:janv]]

Attachment: initial design

rebased patch

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 685593 [details] [diff] [review]
initial design

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

::: dom/indexedDB/ipc/PIndexedDB.ipdl
@@ +14,5 @@
>  namespace indexedDB {
>  
>  protocol PIndexedDB
>  {
> +  manager PBrowser or PContent or PWorker or PWorkers;

Just PWorker.

::: dom/ipc/PWorkers.ipdl
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +rpc protocol PWorkers

I don't think we should have any sync (or rpc) messages in our worker protocols. The sync stuff should be handled in the worker through the use of sync queues.

Also, this should be something like PWorkerManagement or something.

@@ +17,5 @@
> +
> +parent:
> +    async PWorker();
> +
> +    PIndexedDB();

PIndexedDB should only be created from PWorker, not here.

::: dom/ipc/WorkerParent.h
@@ +15,5 @@
> +class WorkerParent : public PWorkerParent,
> +                     public nsISupports
> +{
> +public:
> +    NS_DECL_ISUPPORTS

This doesn't really need to be nsISupports, you just want refcounting, right?  NS_INLINE_DECL_REFCOUNTING

::: dom/webidl/DOMStringList_workers.webidl
@@ +11,5 @@
> + */
> +
> +interface DOMStringList_workers {
> +  readonly attribute unsigned long length;
> +  // XXXvarga getters seem to be not supported in workers

Let's make sure we have a bug filed on this.

::: dom/webidl/IDBFactorySync.webidl
@@ +5,5 @@
> + *
> + * The origin of this IDL file is http://www.w3.org/TR/IndexedDB/
> + */
> +
> +[Constructor]

The factory object should just exist at global scope, you shouldn't need to call 'new IDBFactorySync()'.

::: dom/workers/IDBObjectSync.h
@@ +23,5 @@
> +
> +class IndexedDBRequestWorkerChildBase;
> +class WorkerPrivate;
> +
> +class IDBObjectProxy

I don't really understand the purpose of this class or its derived classes. Why do we need an additional proxy object for each DOM object?

::: dom/workers/RuntimeService.cpp
@@ +1070,5 @@
>    }
>  
> +  mWorkersParent = new WorkersParent();
> +  mIPCThread = new Thread("WorkersIPCThread");
> +  mIPCThread->Start();

How does this thread stop?

::: dom/workers/WorkerScope.cpp
@@ +988,5 @@
>    }
>  
>    // Init other paris-bindings.
>    if (!FileReaderSyncBinding_workers::GetConstructorObject(aCx, global) ||
> +      !IDBFactorySyncBinding_workers::GetProtoObject(aCx, global) ||

You need to add the rest of these here, I think.

Comment 9

[Jan Varga [:janv]]

Attachment: Cleaned up patch (addressed most of the comments)

Comment 10

[Jan Varga [:janv]]

Comment on attachment 703400 [details] [diff] [review]
Cleaned up patch (addressed most of the comments)

I have a new patch with alternative approach to release ipc thread objects.

Comment 11

[Jan Varga [:janv]]

Attachment: patch

Comment 12

[Jan Varga [:janv]]

Attachment: patch v0.1

I extracted IDB parts into a standalone patch which will be attached to bug 798875. So this patch only contains the IPC thread and PWorkerModule/PWorker protocols.

Comment 13

[Jan Varga [:janv]]

Attachment: up to date patch

Comment 14

[Jan Varga [:janv]]

Attachment: patch v0.2

patch for review

Comment 15

[Jan Varga [:janv]]

Attachment: patch v 0.3

Comment 16

[Jan Varga [:janv]]

Attachment: patch v 0.4

This one builds on windows.

Comment 17

[Andrew Overholt [:overholt]]

Is the need for this obviated by PBackground (bug 956218)?

Comment 18

[Jan Varga [:janv]]

(In reply to Andrew Overholt [:overholt] from comment #17)
> Is the need for this obviated by PBackground (bug 956218)?

Well, we'll see once IndexedDB on PBackground is implemented.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we close this bug?  Workers are running the IPC event loop.

Comment 20

[Jan Varga [:janv]]

Yeah, it seems it would be better to update sync IDB to use all the new stuff (new event loop and PBackground) once it's ready, the extra IPC thread would make sense in case we don't have resources to update sync IDB implementation IMO.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm inclined to agree.

Comment 22

[Jan Varga [:janv]]

Ok