Bug 18420 – SquirrelFish: need to throw Reference and Type errors when attempting invalid operations on JSValues

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

Task tracking bug

[Oliver Hunt]

Created attachment 20468 [details]
Here we go

[Geoffrey Garen]

+++ b/JavaScriptCore/VM/ExceptionHelpers.cpp

The machine has a bunch of helper functions right now, all in Machine.cpp. I
think it would be best to keep all the helpers there for now, rather than
splitting out new source files. Especially because Darin will box our ears
if we add a file named "*Helpers" or "*Utilities" to the project :).

+static NEVER_INLINE bool resolve(ExecState* exec, Instruction* vPC, Register* r, ScopeChain* scopeChain, CodeBlock* codeBlock, JSValue*& exceptionData)

I think "excpetionValue" is a clearer name than "exceptionData". It's not an
arbitrary bunch of data -- it's a JSValue*.

+static NEVER_INLINE bool notObject(ExecState* exec, const Instruction*, CodeBlock*, JSValue* value, JSValue*& exceptionData)

How about "isNotObject"?

+        if (UNLIKELY(exec->hadException())) {\

Missing space at the end.

         JSObject* base = r[r1].u.jsValue->toObject(exec);
-        // FIXME: missing exception check
+        
         JSValue* subscript = r[r2].u.jsValue;
 
         uint32_t i;
         if (subscript->getUInt32(i))
             r[r0].u.jsValue = base->get(exec, i);
-        else
+        else {
+            VM_CHECK_EXCEPTION(); // This is needed as toString may have side effects
             r[r0].u.jsValue = base->get(exec, Identifier(subscript->toString(exec)));
             
I think I only understand this comment because you explained what was going on
to me in person. How about this:

    JSObject* base = r[r1].u.jsValue->toObject(exec); // may throw
    ...
    else {
        VM_CHECK_EXCEPTION(); // If toObject threw, we must not call toString, which may execute arbitrary code
        r[r0].u.jsValue = base->get(exec, Identifier(subscript->toString(exec)));
    
+        else {
+            VM_CHECK_EXCEPTION(); // This is needed as toString may have side effects
             base->put(exec, Identifier(subscript->toString(exec)), r[r2].u.jsValue);
-
+        }

Ditto.

+        else {
+            VM_CHECK_EXCEPTION(); // This is needed as toString may have side effects
             r[r0].u.jsValue = jsBoolean(base->deleteProperty(exec, Identifier(subscript->toString(exec))));
             
Ditto.

+        // FIXME throw type error
         ASSERT(callType == CallTypeNone);
-        ++vPC;
+
+        exit(0);
+        vPC++;

Ummm... yeah.... no :).

-        // FIXME: We need to throw a TypeError here if v is not an Object.
-        ASSERT(v->isObject());
-
+        ASSERT(callType == CallTypeNone || v->isObject());

Seems like overkill to me, but okay.

         ASSERT(callType == CallTypeNone);
+        
+        if (notObject(exec, vPC, codeBlock, v, exceptionValue))
+            goto internal_throw;
 
+        // throw type error for non-contructor object
+        ASSERT(!constructor->implementsConstruct()); // if we get here then v must be an Object that is not a constructor

I'm a little confused by the above. It seems no matter how we got here, we
should throw a "You can't use X with 'new'" exception. No?

+        exceptionTarget = throwException(exec, exceptionValue, codeBlock, k, scopeChain, registerBase, r, vPC);
+        if (!exceptionTarget) {
+            registerFile->shrink(0);
+            return exceptionValue;
+        }

No shrink-y, please.

+    JSValue *InvalidObject::toPrimitive(ExecState *exec, JSType) const
+    {
+        UNUSED_PARAM(exec);
+        ASSERT(exec->hadException() && exec->exception() == m_exception);
+        return m_exception;
+    }

I don't think this is valid. ToPrimitive is required to return a primitive
type. Or does it all work out in the end?

+    JSObject *InvalidObject::toObject(ExecState *exec) const

What's with all the "ExecState *"? Don't you know that the WebKit project was
started by a bunch of guys who dreamed to move the "*" on every line of KHTML
ever written?

+namespace KJS {
+    class InvalidObject : public JSObject {

You're going to think I'm crazy, but hear me out: Let's call this class
"JSNotAnObject". I know what you're thinking: "How could JSNotAnObject be a
subclass of JSObject?" But, the thing is, in JavaScript, something that is not
an object can be used as if it were an object, and that's how this object comes
about! For example:

(1).x // 1 (not an object) used as an object

So, here, 1 "is a" object, to some extent. (Until it throws, anyway).

Also might be worth a one-line comment at the top of the file explaining why
this unholy object exists.

r=me, but definitely no shrink-y and no exit-y, please.

[Oliver Hunt]

Committed r31798