Bug 36594

Created attachment 51631 [details] Fix v1

Comment on attachment 51631 [details] Fix v1 > +QScriptValuePrivate* QScriptValuePrivate::toObject(QScriptEnginePrivate* engine) > +{ > + switch (m_state) { > + case Invalid: > + return new QScriptValuePrivate; > + case CString: > + case CNumber: > + case CBool: > + case CSpecial: > + assignEngine(engine); > + // Now m_state == JSPrimitive so fall-through. Instead of a comment I'd put an assert there, since it gives you the same information but actually enforces it. > + case JSValue: > + case JSPrimitive: > + { > + if (engine != this->engine()) { > + qWarning("Do not use different engines for the same value"); > + // We can solve the problem by using a bounded engine instead of the given one, > + // but still it should not happen, this is a bad usage of the QScriptEngine::toObject() > + } There doesn't seem to be any test for the engine != this->engine() case. > + QScriptValue object = eng.evaluate("new Object"); > + { > + QScriptValue tmp = object.toObject(); > + QCOMPARE(tmp.isObject(), true); > + } There should be a tmp.strictlyEquals(object) check. There should also be checks to make sure that QScriptEngine::toObject() doesn't change the type of the value argument, and that QScriptValue::toObject() doesn't change the type of the this-JS-value. I think those tests would fail with this patch since QScriptValuePrivate::toObject() not only does the conversion but also changes the type.

Created attachment 53564 [details] Fix v2 It should be much better now. Kent what do you think about it ?

(In reply to comment #3) > Created an attachment (id=53564) [details] > Fix v2 > > It should be much better now. Kent what do you think about it ? Something to be aware of: In Qt 4.5 and 4.6 we allow using QScriptEngine::toObject() on a value constructed in another engine. It's not tested in the autotests, but it works (granted, more by "luck" than by design). Ideally I'd like to have the check, but since it's a difference in behavior, we should consider it. Should 4.6/4.7 be changed to do the check as well? (Because nobody would ever rely on this behavior, right...? ;) ) Declare the lack-of-check a bug, that is? (I think it is, it's just that it's been that way forever...) If the check remains, the qWarning message at least needs to be qualified, e.g. "QScriptEngine::toObject: cannot convert value created in a different engine"; this also makes the overall wording consistent with similar warnings in other API. It would be good if you could add QVERIFYs to the tests to ensure that the type of the original value has not changed after the toObject() call (I've already added such checks to the tests in Qt).

Created attachment 53860 [details] Fix v2 > Something to be aware of: In Qt 4.5 and 4.6 we allow using > QScriptEngine::toObject() on a value constructed in another engine. It's not > tested in the autotests, but it works (granted, more by "luck" than by design). > > Ideally I'd like to have the check, but since it's a difference in behavior, we > should consider it. Should 4.6/4.7 be changed to do the check as well? (Because > nobody would ever rely on this behavior, right...? ;) ) Declare the > lack-of-check a bug, that is? (I think it is, it's just that it's been that way > forever...) Who can rely on this? Of course nobody :-) I think that if it works we should keep it, but not support it. One of the main rules of QtScript is that developers can't mix engines, values created in one engine couldn't be transfered to an another and so on... It make sens for me to add the qWarning to current Qt without forcing the behavior. > If the check remains, the qWarning message at least needs to be qualified, e.g. > "QScriptEngine::toObject: cannot convert value created in a different engine"; > this also makes the overall wording consistent with similar warnings in other > API. Changed. > It would be good if you could add QVERIFYs to the tests to ensure that the type > of the original value has not changed after the toObject() call (I've already > added such checks to the tests in Qt). Done.

(In reply to comment #5) > Created an attachment (id=53860) [details] > Fix v2 Looks good.

Comment on attachment 53860 [details] Fix v2 Nice to see that the new QtScript implementation is moving along nicely :-)

Comment on attachment 53860 [details] Fix v2 Clearing flags on attachment: 53860 Committed r59503: <http://trac.webkit.org/changeset/59503>

All reviewed patches have been landed. Closing bug.