Closed Bug 812783 Opened 12 years ago Closed 12 years ago

Add sane support for worker-only callbacks

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: janv, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

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.
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); ... }
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
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.
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...
Attached patch hacky fix (deleted) — Splinter Review
Attachment #683022 - Attachment mime type: application/x-fossil-artifact → text/plain
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
Attachment #683097 - Flags: review?(khuey)
Assignee: nobody → bzbarsky
Keywords: dev-doc-needed
Whiteboard: [need review]
the last patch fixes the problem for me
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla20
Status: NEW → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: