JUCE 8 JavascriptEngine DynamicObject::hasMethod/invokeMethod Broken

General JUCE discussion
1

We have some custom behaviour around our native objects used in javascript so we create a juce::DynamicObject subclass and override hasMethod/invokeMethod to do what we need.

However, it seems like these are never queried in JUCE 8 so our methods just aren’t registered with the property and silently won’t get called.

I do have a workaround where I can manually call juce::DynamicObject::setMethod in the subclass’s constructor and forward to the old implementations but this is a fairly big change of behaviour.

I’ve not looked in to a fix but presumably when the DynamicObject’s member’s are queried, the object needs to queried as well?

I can provide test code if required.

(JUCE 8) QuickJS implementation missing methods
2

Thanks for reporting, this issue should now be resolved:

3

Unfortunately not. I don’t think this update addresses this issue at all.
It’s about overriding DynamicObject::hasMethod/invokeMethod.

Here’s an example test case:

void testOverridingHasInvokeMethod()
{
    struct DerivedDynamicObject : public DynamicObject
    {
        bool hasMethod (const Identifier& methodName) const override
        {
            return methodName.toString() == "testMethod";
        }

        juce::var invokeMethod (Identifier methodName, const var::NativeFunctionArgs&) override
        {
            if (methodName.toString() == "testMethod")
                return 42;

            return {};
        }
    };

    auto createEngine = [&]
    {
        auto engine = std::make_unique<juce::JavascriptEngine>();
        engine->registerNativeObject ("DerivedNativeObject", new DerivedDynamicObject());

        return engine;
    };

    auto engine = createEngine();
    auto res = juce::Result::fail ("");
    auto val = engine->evaluate ("let derivedObj = DerivedNativeObject; \
                                  derivedObj.testMethod();",
                                 &res);
    jassert (res.wasOk());
    jassert (static_cast<int> (val) == 42);
}

If you put a breakpoint in hasMethod/invokeMethod, you’ll see they don’t ever get called. I can’t see from the code where they are called either?

4

I see, sorry about the misunderstanding. Thanks for the example code.

Unfortunately I’m not sure whether this functionality can be made to work with the QuickJS engine. As far as I can tell, methods must be explicitly registered by name. There doesn’t seem to be any API to intercept method calls during the name lookup.

I think this would be the way to go if there were some way to list all the methodNames that invokeMethod intends to check, but we don’t have anything like that currently. The problem is that in the example you posted, there’s no way to determine that the object has a method named “testMethod” other than passing this string to hasMethod, because the name “testMethod” isn’t included in the result of getProperties(). This means we can’t e.g. loop through all properties and register native functions that forward to invokeMethod.

Looking at this a bit more, I’m not sure that it makes sense for DynamicObject’s hasProperty, getProperty, setProperty, removeProperty, hasMethod, and invokeMethod to be virtual, because this makes it very easy to break the type’s invariants. I don’t think it should be possible to change the behaviour of these functions to be inconsistent with the result of getProperties(). It’s surprising that myObj->invokeMethod ("foo", args) may produce different results to myObj->getProperties()["foo"].getNativeFunction() (args).

How big of a breaking change would it be for Waveform if we were to make these members non-virtual? Do you have any functionality that depends on method dispatching inside invokeMethod, such as methods that are conditionally enabled?

5

We could adapt to having to register the methods, I’ve got this working now.

I tend to agree that having these as virtual functions doesn’t really make sense but it was the fact that it was a silent breaking change that was difficult for us. I’d be happy if they were just removed, we can then always remove the override specifier and just forward calls on to captured references.

Tbh I’m not quite sure what the difference between hasMethod/hasProperty etc. is? Aren’t methods just callable properties in JS?

6

Great! In that case I’ll consider making the functions non-virtual and updating the breaking-changes doc.

That’s right, it looks like hasProperty returns false for methods but true for all other kinds of property, while hasMethod only returns true for methods.

7

This change is now in place on the develop branch:

8

Thanks. We’ve got this implemented now.

We do have some UB triggering with QuickJS but it’s a bit hard to narrow down why… I’ll try and write up a report for that soon.

1 Like