Add sane support for worker-only callbacks

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: janv, Assigned: bzbarsky)

Tracking

({dev-doc-complete})

unspecified
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Reporter

Description

7 years ago
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#6692

# Do codegen for all the callbacks.  Only do non-worker codegen for now,
# since we don't have a sane setup yet for invoking callbacks in workers
# and managing their lifetimes.

We need this for bug 798875.
Reporter

Comment 1

7 years ago
Boris, please let us know if there's a temp workaround.

here's the WebIDL:

interface IDBTransactionSync {
  ...
};

callback IDBTransactionCallback = void(IDBTransactionSync transaction);

interface IDBDatabaseSync {
  ...
  void
  transaction(any storeNames,
              IDBTransactionCallback callback,
              optional DOMString mode,
              optional unsigned long timeout);
  ...
}
Reporter

Updated

7 years ago
Blocks: SyncIDB
That IDL should work just fine in workers.  We don't generate the CallbackFunction objects there, because workers are not cycle collected, but we do pass through the callable JSObject*.

So what you can do for now is just store that object, do whatever GC tracing you have to to keep it alive, and call it as needed.  Now the good news is that in worker code most of the complexity of calling JS on the main thread (multiple JSContexts, security, etc) is just not an issue.  You can just go and JS_CallFunctionValue as needed.  You won't have to do return value conversion, and converting an IDBTransactionSync to a JS::Value should be pretty straightforward, I would think.

As for adding codegen support for things like the argument and return value conversion, encapsulated in some object, that depends on a clear explanation of how the ownership model for this object should work.  For one thing, should the worker code hold some object that then holds a JSObject, or hold a JSObject and just call a static method that does the call on that JSObject?
Component: DOM → DOM: Workers
Reporter

Comment 3

7 years ago
Ok, but the WebIDL doesn't compile at the moment, I get this error:

Traceback (most recent call last):
  File "/Users/varga/Sources/Sync/mozilla-central/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/Users/varga/Sources/Sync/mozilla-central/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/BindingGen.py", line 69, in <module>
    main()
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/BindingGen.py", line 62, in main
    generate_binding_header(config, outputPrefix, webIDLFile);
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/BindingGen.py", line 20, in generate_binding_header
    root = CGBindingRoot(config, outputprefix, webidlfile)
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/Codegen.py", line 6600, in __init__
    iface in ifaces)
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/Codegen.py", line 6600, in <genexpr>
    iface in ifaces)
  File "/Users/varga/Sources/Sync/mozilla-central/dom/bindings/Configuration.py", line 125, in getDescriptor
    str(len(matches)) + " matches");
Configuration.NoSuchDescriptorError: For IDBTransactionSync found 0 matches

when I add 'skipGen' to Bindings.conf:
'IDBTransactionSync': [
{                   
    'skipGen': True 
},                  
{                   
    'workers': True,
}],

I get this error:
/Users/varga/Sources/Sync/obj-ff-dbg/dom/bindings/IDBTransactionCallbackBinding.cpp:11:10: fatal error: 
      'mozilla/dom/IDBTransactionSync.h' file not found
#include "mozilla/dom/IDBTransactionSync.h"
         ^
1 error generated.
make[1]: *** [IDBTransactionCallbackBinding.o] Error 1

Here's a snippert from IDBTransactionCallbackBinding.h:
namespace mozilla {
namespace dom {

class IDBTransactionCallback : public CallbackFunction
{
public:
  inline IDBTransactionCallback(JSContext* cx, JSObject* aOwner, JSObject* aCallable, bool* aInited)
    : CallbackFunction(cx, aOwner, aCallable, aInited)
  {
  }
  
  template <typename T>
  inline void
  Call(const T& thisObj, mozilla::dom::IDBTransactionSync& transaction, ErrorResult& aRv)
  {

I don't know if it is possible to force the code generator to use mozilla::dom::workers::IDBTransactionSync in the Call() method.
Reporter

Comment 4

7 years ago
or you mean to remove the IDBTransactionCallback interface from WebIDL and just call JS_CallFunctionValue() on the JSObject ?

for example, here's the method where the callback is passed:

void
IDBDatabaseSync::Transaction(JSContext* aCx, JS::Value aStoreNames,
                             NonNull<JSObject>& aCallback,
                             const Optional<nsAString>& aMode,
                             const Optional<uint32_t>& aTimeout)
{
  ...
  JS_CallFunctionValue(..., aCallback, ...);
  ...
}
Oh, I see.  The problem is that the codegen for the non-worker version of this callback fails, right.

So I think short-term you should just add 

  'nativeType': mozilla::dom::workers::IDBTransactionSync

to the non-worker descriptor with "'skipGen': True" that you added for IDBTransactionSync.  That should at least get you building.

Longer-term, I think we should add a [WorkerOnly] annotation for callbacks that can be used to indicate that main-thread codegen should be skipped entirely for that callback.  Peter, does that seem reasonable?

For this specific case we could also remove the check for descriptorProvider.workers in CGCallbackFunction.__init__, but then there will be no easy way to figure out that a callback is failing to be generated because someone screwed up Bindings.conf somewhere...
Reporter

Comment 6

7 years ago
Posted patch hacky fixSplinter Review
Reporter

Updated

7 years ago
Attachment #683022 - Attachment mime type: application/x-fossil-artifact → text/plain
Reporter

Comment 10

7 years ago
The patch was made on top of the "Initial WebIDL" patch in bug 798875.
I had to add other stuff to Bindings.conf to make it "almost" build.
The other problem is that when "skipGen" is set for non-worker IDBTransactionSync then the code generator generates WrapObject() for it in the Call() method.
And then I get this error:

/Users/varga/Sources/Sync/obj-ff-dbg/dom/bindings/IDBTransactionCallbackBinding.cpp:32:10: error: 
      no matching function for call to 'WrapObject'
    if (!WrapObject(cx, mCallable, transaction, &argv[0])) {
         ^~~~~~~~~~
../../dist/include/mozilla/dom/BindingUtils.h:790:1: note: candidate template
      ignored: failed template argument deduction
WrapObject(JSContext* cx, JSObject* scope, T* p, JS::Value* vp)
^
../../dist/include/mozilla/dom/BindingUtils.h:807:1: note: candidate template
      ignored: failed template argument deduction
WrapObject(JSContext* cx, JSObject* scope, const nsCOMPtr<T> &p, JS::Value* vp)
^
../../dist/include/mozilla/dom/BindingUtils.h:824:1: note: candidate template
      ignored: failed template argument deduction
WrapObject(JSContext* cx, JSObject* scope, const nsRefPtr<T> &p, JS::Value* vp)
^
../../dist/include/mozilla/dom/BindingUtils.h:765:1: note: candidate function
      template not viable: requires 6 arguments, but 4 were provided
WrapObject(JSContext* cx, JSObject* scope, T* p, nsWrapperCache* cache,
^
../../dist/include/mozilla/dom/BindingUtils.h:779:1: note: candidate function
      template not viable: requires 5 arguments, but 4 were provided
WrapObject(JSContext* cx, JSObject* scope, T* p, const nsIID* iid,
^
../../dist/include/mozilla/dom/BindingUtils.h:798:1: note: candidate function
      template not viable: requires 5 arguments, but 4 were provided
WrapObject(JSContext* cx, JSObject* scope, const nsCOMPtr<T> &p, const ...
^
../../dist/include/mozilla/dom/BindingUtils.h:815:1: note: candidate function
      template not viable: requires 5 arguments, but 4 were provided
WrapObject(JSContext* cx, JSObject* scope, const nsRefPtr<T> &p, const ...
^
1 error generated.

I could add another WrapObject() method (that takes a ref, not pointer) to BindingUtils.h, but if I understood it correctly, WrapObject() is for objects which haven't been converted yet.
So I removed the "and not descriptor.skipGen" check and now everything builds.
Not sure if it affects something else, it seems it's currently used only for the "Document"
OK.  Let me write up WorkerOnly thing, I guess....
Summary: Add codegen support for callbacks in workers → Add sane support for worker-only callbacks
Assignee: nobody → bzbarsky
Keywords: dev-doc-needed
Whiteboard: [need review]
Reporter

Comment 13

7 years ago
the last patch fixes the problem for me
https://hg.mozilla.org/integration/mozilla-inbound/rev/a56c5c4bb2bd
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/a56c5c4bb2bd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Note that I'm planning to rip out the bits we added here in favor of the better setup from bug 830099.  It shouldn't affect bug 798875, I hope, except for needing to remove the [WorkerOnly] annotation.
You need to log in before you can comment on or make changes to this bug.