Bug 40060 – Add a version of JSObjectSetPrototype that yields an exception when the prototype can't be set

• Component: JavaScriptCore
• Status: NEW

[Jędrzej Nowacki]

A try to create cycle in a prototype chain will end up in an exception

> a = new Object
[object Object]
> a.__proto__=a
Exception: Error: cyclic __proto__ value
>

This is not true in the JSC C API. Invalid call to the JSObjectSetPrototype will be just ignored. It would be nice to have an unified interface here. I think that the JSObjectSetPrototype function should take one parameter more (an exception handler), but as it is not possible without breaking binary compatibility, a new function should be created.

This bug is a next step for bug 39360.

[Jędrzej Nowacki]

Created attachment 57654 [details]
Fix v1

[Darin Adler]

Comment on attachment 57654 [details]
Fix v1

> +void JSObjectSetPrototypeEx(JSContextRef ctx, JSObjectRef object, JSValueRef value, JSValueRef* exception)

You need to discuss the naming of this with Geoff Garen. The "Ex" suffix is almost certainly *not* what we want to do here.

[Darin Adler]

Comment on attachment 57654 [details]
Fix v1

I’m not sure the C API needs this feature. What’s the reason we need to add this? Is someone asking for it?

[Eric Seidel (no email)]

Comment on attachment 57654 [details]
Fix v1

r- per above naming comment.

[Jędrzej Nowacki]

(In reply to comment #2)
> (From update of attachment 57654 [details])
> > +void JSObjectSetPrototypeEx(JSContextRef ctx, JSObjectRef object, JSValueRef value, JSValueRef* exception)
>
> You need to discuss the naming of this with Geoff Garen. The "Ex" suffix is almost certainly *not* what we want to do here.
It is why, he is in CC :-)
I don't know Mac naming policy, I agree that the name could be better. In C++ I could use function overloading, but it is a C code. Do you have idea for a better name?

(In reply to comment #3)
> (From update of attachment 57654 [details])
> I’m not sure the C API needs this feature. What’s the reason we need to add this? Is someone asking for it?
Yes, I'm asking for it :-).
1. I think there is an inconsistency; in the jsc console creating cycle in a prototype chain results in an exception in C API these exception is ignored.
2. I would like to implement fix for bug 39356, current QtScript implementation shows a warning when an user tries to create a cyclic prototype chain.

[Geoffrey Garen]

> > > +void JSObjectSetPrototypeEx(JSContextRef ctx, JSObjectRef object, JSValueRef value, JSValueRef* exception)
> >
> > You need to discuss the naming of this with Geoff Garen. The "Ex" suffix is almost certainly *not* what we want to do here.

I agree that the "Ex" naming isn't good. It doesn't say what's different about the "Ex" version, and it only works once. If we make another mistake, will we name the next function "ExEx?"

> > I’m not sure the C API needs this feature. What’s the reason we need to add this? Is someone asking for it?
> Yes, I'm asking for it :-).

I'm not convinced the C API needs this feature, either.

Bug 39360 was clearly an improvement -- it's nice when the API can prevent you from shooting yourself in the foot without undue performance cost.

However, to have two versions of the same function, I think the second version really needs to justify itself. I believe you can get the behavior you want for bug 39356 using the current API, by either:

(a) setting the "__proto__" property using JSObjectSetProperty.

or

(b) using JSObjectGetPrototype in a loop to check for cycles yourself.

[Jędrzej Nowacki]

Created attachment 59259 [details]
Fix v1.1

(In reply to comment #6)
> > > > +void JSObjectSetPrototypeEx(JSContextRef ctx, JSObjectRef object, JSValueRef value, JSValueRef* exception)
> > >
> > > You need to discuss the naming of this with Geoff Garen. The "Ex" suffix is almost certainly *not* what we want to do here.
>
> I agree that the "Ex" naming isn't good. It doesn't say what's different about the "Ex" version, and it only works once. If we make another mistake, will we name the next function "ExEx?"
Oh, then we will use "ExBis" :-) I do know that "Ex" is wrong but which name would be right? What about JSObjectSetPrototypeWithErrorHandling? I'm attaching rebased patch.

> > > I’m not sure the C API needs this feature. What’s the reason we need to add this? Is someone asking for it?
> > Yes, I'm asking for it :-).
> I'm not convinced the C API needs this feature, either.
> (...)
> However, to have two versions of the same function, I think the second version really needs to justify itself.
> (...)

This function can stay in a private header for now, for example next version of JSC API and then it could replace the original one. I don't insist to create an officially supported API, I'm fine with binary compatibility breakages or even small refactoring as long as I can access to the functionality with good performance. Is API's performance a good justification to introduce the function?

I wrote 5 implementation of the QScriptValue::setPrototype(). I test them using the same benchmark:

  QScriptEngine engine;
  QScriptValue value = engine.newObject();
  QScriptValue proto1 = engine.newObject();
  QScriptValue proto2 = engine.newObject();
  QScriptValue tmp = engine.newObject();
  tmp.setPrototype(engine.newObject());
  proto2.setPrototype(tmp);

  QBENCHMARK {
      value.setPrototype(proto1);
      value.setPrototype(proto2);
  }

The benchmark simply call the setPrototype function changing a prototype for the value variable,
proto1 represent a short prototype chain (0 element above proto1),
proto2 represent a longer prototype chain (2 elements above proto2).
QBENCHMARK macro is a part of Qt benchmarking tool.

#if 1
// This solution use a new API.

        JSValueRef exception = 0;
        JSObjectSetPrototypeWithErrorHandling(*m_engine, *this, *prototype, &exception);
        if (exception)
            qWarning("QScriptValue::setPrototype() failed: cyclic prototype value");

//RESULT : tst_QScriptValue::setProtoBench():
//     0.00047 msecs per iteration (total: 62, iterations: 131072)

#elif 0

// This solution tries to set the __proto__ property

        JSValueRef exception = 0;
        JSRetainPtr<JSStringRef> protoName = JSStringCreateWithUTF8CString("__proto__");
        JSObjectSetProperty(*m_engine, *this, protoName.get(), *prototype, /* attr */ 0, &exception);
        if (exception)
            qWarning("QScriptValue::setPrototype() failed: cyclic prototype value");
            
//RESULT : tst_QScriptValue::setProtoBench():
//     0.0012 msecs per iteration (total: 80, iterations: 65536)
     
#elif 0

// This solution iterates over value's prototype chain to check if an error can occur.

        JSValueRef proto = *prototype;
        while (!JSValueIsNull(*m_engine, proto)) {
            if (JSValueIsStrictEqual(*m_engine, proto, *this)) {
                qWarning("QScriptValue::setPrototype() failed: cyclic prototype value");
                return;
            }
            JSValueRef exception = 0;
            proto = JSObjectGetPrototype(*m_engine, JSValueToObject(*m_engine, proto, &exception));
            if (exception) {
                qWarning("OOPS");
                return;
            }
        }
        JSObjectSetPrototype(*m_engine, *this, *prototype);

//RESULT : tst_QScriptValue::setProtoBench():
//     0.0036 msecs per iteration (total: 60, iterations: 16384)

#elif 0

//This solution is same as previous one, but it use a trick to avoid redundant conversion from
//JSValueRef to JSObjectRef.

        JSValueRef proto = *prototype;
        while (!JSValueIsNull(*m_engine, proto)) {
            if (JSValueIsStrictEqual(*m_engine, proto, *this)) {
                qWarning("QScriptValue::setPrototype() failed: cyclic prototype value");
                return;
            }
            proto = JSObjectGetPrototype(*m_engine, const_cast<JSObjectRef>(proto));
        }
        JSObjectSetPrototype(*m_engine, *this, *prototype);

//RESULT : tst_QScriptValue::setProtoBench():
//     0.0028 msecs per iteration (total: 92, iterations: 32768)
     
#else

//This solution is based on implementation detail, I know that only possible error is the "cyclic prototype value",
//so if the JSObjectSetPrototype fails to set a prototype then I know what happened.

        JSObjectSetPrototype(*m_engine, *this, *prototype);
        JSValueRef proto = JSObjectGetPrototype(*m_engine, *this);
        if (!JSValueIsStrictEqual(*m_engine, proto, *prototype)) {
            qWarning("QScriptValue::setPrototype() failed: cyclic prototype value");
        }

//RESULT : tst_QScriptValue::setProtoBench():
//     0.00091 msecs per iteration (total: 60, iterations: 65536)

#endif


The first solution is the shortest and the fastest one (2 - 8x faster then others).

[Geoffrey Garen]

> > I agree that the "Ex" naming isn't good. It doesn't say what's different about the "Ex" version, and it only works once. If we make another mistake, will we name the next function "ExEx?"
> Oh, then we will use "ExBis" :-) I do know that "Ex" is wrong but which name would be right?

Something that denoted the difference between the two functions.

> What about JSObjectSetPrototypeWithErrorHandling?

JSObjectSetPrototypeWithException would probably be better. The function doesn't actually handle errors -- it just supports an exception out parameter.

> This function can stay in a private header for now, for example next version of JSC API and then it could replace the original one.

I don't think it's practical to replace the old function, since that would break binary compatibility.

> Is API's performance a good justification to introduce the function?

It can be. It depends on whether this is a performance-critical path. Is it? If so, why?

[WebKit Review Bot]

Attachment 59259 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3299550

[Jędrzej Nowacki]

Created attachment 59358 [details]
Fix v1.2

(In reply to comment #8)
> JSObjectSetPrototypeWithException would probably be better. The function doesn't actually handle errors -- it just supports an exception out parameter.
Thanks, changed.

> > This function can stay in a private header for now, for example next version of JSC API and then it could replace the original one.
> I don't think it's practical to replace the old function, since that would break binary compatibility.
You mean that we won't be able to break the binary compatibility promise, never?

> > Is API's performance a good justification to introduce the function?
> It can be. It depends on whether this is a performance-critical path. Is it? If so, why?
As the QtScript is an API, I have a similar problem, like the JSC C API, to really define a good use cases. Lets try a logical experiment. Could you give me a performance-critical path for setting JSObjectSetProperty? If yes then it is the same as mine :-) if not can I put in the implementation a sleep() call? I try to make implementation of the QtScript clean and fast as possible. As you pointed me, there are a few ways to get the behavior, but I believe that, they are not so clean and fast as they could be. If you could create the JSObjectSetPrototype function again, assuming that you would knew about the exception, would you create it with an exception parameter or without? I think, it should be fixed, but I don't want to push to much as I can live without it (2x time longer as it is as much slower :-) ).

[WebKit Review Bot]

Attachment 59358 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3325568

[Jędrzej Nowacki]

(In reply to comment #11)
> Attachment 59358 [details] did not build on win:
> Build output: http://webkit-commit-queue.appspot.com/results/3325568
How to download the build log ?

[Darin Adler]

Geoff, I think you should make the call on this. The mechanics of the patch look OK to me.

[Geoffrey Garen]

> > I don't think it's practical to replace the old function, since that would break binary compatibility.
> You mean that we won't be able to break the binary compatibility promise, never?

Correct. That's one reason we're particularly conservative about API.

> Lets try a logical experiment. Could you give me a performance-critical path for setting JSObjectSetProperty? If yes then it is the same as mine :-) if not can I put in the implementation a sleep() call?

This experiment didn't persuade me, since there's no clear benefit to adding a sleep() call to JSObjectSetProperty().

> If you could create the JSObjectSetPrototype function again, assuming that you would knew about the exception, would you create it with an exception parameter or without?

If I had it all to do over again, I would either add the exception parameter or remove all exception parameters and make pending exceptions an implicit property of the execution context.

[Geoffrey Garen]

Comment on attachment 59358 [details]
Fix v1.2

I don't think we ever want to make JSObjectSetPrototypeWithException public API.

However, I don't see any downside to adding it as private API -- it doesn't introduce any new dependencies on the VM.

So, let's compromise by adding this as private API, which Qt library implementors can use, with the understanding that we're not going to make it into public API.

[WebKit Commit Bot]

Comment on attachment 59358 [details]
Fix v1.2

Rejecting patch 59358 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
ompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Debug/jsc.build/Objects-normal/i386/jsc.o /Users/eseidel/Projects/CommitQueue/JavaScriptCore/jsc.cpp normal i386 c++ com.apple.compilers.gcc.4_2
testapi:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Debug/testapi.build/Objects-normal/i386/testapi.o /Users/eseidel/Projects/CommitQueue/JavaScriptCore/API/tests/testapi.c normal i386 c com.apple.compilers.gcc.4_2
(68 failures)


Full output: http://queues.webkit.org/results/3923394

[Eric Seidel (no email)]

Comment on attachment 59358 [details]
Fix v1.2

It seems this would break Mac, based on the commit-queue failure.