[webkit-reviews] review denied: [Bug 17228] console.{log, warn, info, error} should support format strings, variable arguments : [Attachment 19946] Step 3: Pass all arguments to console.{log, warn, info, error} down to the Inspector's JS

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Roben (aroben)
<aroben at apple.com>'s request for review:
Bug 17228: console.{log,warn,info,error} should support format strings,
variable arguments
http://bugs.webkit.org/show_bug.cgi?id=17228

Attachment 19946: Step 3: Pass all arguments to console.{log,warn,info,error}
down to the Inspector's JS
http://bugs.webkit.org/attachment.cgi?id=19946&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think it could be unsafe to pass the actual JavaScript objects to the
inspector. This allows a webpage to get the inspector to execute code on its
behalf in the inspector's privileged context, for example if it supplies an
object with a custom toString function.

+	 JSValueRef messageValue = JSValueMakeString(m_scriptContext,
messageString.get());
+	 arguments.append(messageValue);

This is a problem. Once the local variable messageValue goes out of scope, the
string could be garbage collected. Since you're using a Vector here, the values
won't be on the stack and won't be seen by the garbage collector. Depending on
how the optimizer optimizes the C++ code, this could be an issue for the other
things like sourceValue as well; the compiler is not obligated to keep the
value around on the stack or in a register once you've made the last use of it.


Because of this, I think you need either not use a Vector or use something that
calls JSValueProtect.

My suggestion is that you choose a fixed maximum number of arguments and use an
array on the stack rather than a Vector.

review- because of the GC issue with the arguments Vector.