Closed
Bug 812783
Opened 12 years ago
Closed 12 years ago
Add sane support for worker-only callbacks
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: janv, Assigned: bzbarsky)
References
Details
(Keywords: dev-doc-complete)
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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•12 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);
...
}
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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•12 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, ...);
...
}
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
Reporter | ||
Comment 7•12 years ago
|
||
Reporter | ||
Comment 8•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #683022 -
Attachment mime type: application/x-fossil-artifact → text/plain
Reporter | ||
Comment 9•12 years ago
|
||
Reporter | ||
Comment 10•12 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"
Assignee | ||
Comment 11•12 years ago
|
||
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 | ||
Comment 12•12 years ago
|
||
Attachment #683097 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Reporter | ||
Comment 13•12 years ago
|
||
the last patch fixes the problem for me
Attachment #683097 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 14•12 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla20
Assignee | ||
Comment 15•12 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
Comment 16•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•12 years ago
|
||
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.
Description
•