Bug 17228 – console.{log,warn,info,error} should support format strings, variable arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Adam Roben (:aroben)]

Our implementation of console.{log,warn,info,error} should support format strings, like Firebug does.

[Adam Roben (:aroben)]

<rdar://problem/5732828>

[Adam Roben (:aroben)]

Firebug also lets you list more variables than you have format specifiers, and the remaining variables are appended to the string separated by spaces.

[Adam Roben (:aroben)]

Created attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified

[Adam Roben (:aroben)]

Created attachment 19933 [details]
Step 2: Route all Console-related activities through Console

[Adam Roben (:aroben)]

Created attachment 19934 [details]
Step 2: Route all Console-related activities through Console

Corrected a typo

[Adam Roben (:aroben)]

Created attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Uploaded the right patch this time

[Adam Roben (:aroben)]

Created attachment 19937 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

[Adam Roben (:aroben)]

Created attachment 19946 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

We're now more careful in how we handle the arguments passed in from the inspected page.

[Darin Adler]

Comment on attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified

This is unconventional structure. It's not really a "binding" if we're passing the JavaScript arguments directly to the underlying DOM object.

I'm going to say r=me, but if console is really going to be a pure JavaScript object, then maybe we shouldn't use .idl to create the object.

[Darin Adler]

Comment on attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Why does the console() hang off domWindow()? Why not directly off Frame?

I think we should move the "shouldPrintExceptions" and printf stuff into the console object too.

+void FrameLoader::reportLocalLoadFailed(const Frame* frame, const String& url)

The use of const Frame* here is strange. Just Frame* will do.

-        void addMessageToConsole(MessageSource, MessageLevel, const String& message, unsigned lineNumber, const String& sourceID);
+        // This method should only be called by Console
+        void addMessageToConsole(const String& message, unsigned lineNumber, const String& sourceID);

I think Console should talk directly to the ChromeClient. I don't think we need a Chrome function in between. Then you wouldn't need that comment. Also, please don't call C++ member functions "methods".

It's extremely unconventional to use the name "state" for an ExecState* parameter. We use "exec" almost everywhere. I think "state" might be better if we did it everywhere, but I think it's not a great idea to start being original about this in the console code.

r=me

[Darin Adler]

Comment on attachment 19946 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

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.

[Adam Roben (:aroben)]

(In reply to comment #9)
> (From update of attachment 19931 [details] [edit])
> This is unconventional structure. It's not really a "binding" if we're passing
> the JavaScript arguments directly to the underlying DOM object.

I suppose that's true. Sam was saying he thought we should leave the string-only versions of the functions as well so that they we can make them callable from, e.g., Obj-C. I decided not to leave them in this patch because we aren't currently using them, and I didn't want to leave in dead code.

> I'm going to say r=me, but if console is really going to be a pure JavaScript
> object, then maybe we shouldn't use .idl to create the object.

I guess if we do add back the string-only versions of the functions then Console will be more of a "true" binding again.

[Adam Roben (:aroben)]

Created attachment 20561 [details]
Step 3 (v2): Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

This patch addresses Darin's comments about security and garbage collection issues.

[Adam Roben (:aroben)]

Created attachment 20566 [details]
Step 4: Separate out tokenizing of format strings

This is the first in a series of patches that refactors our string formatting code so that it can be used by the Console.

[Adam Roben (:aroben)]

Created attachment 20569 [details]
Step 5: Make the formatting functions used for each format specifier configurable

[Adam Roben (:aroben)]

Created attachment 20570 [details]
Step 6: Make String.format able to produce objects other than strings

[Adam Roben (:aroben)]

Created attachment 20571 [details]
Step 7: Return the unused substitutions from String.format

[Adam Roben (:aroben)]

Created attachment 20573 [details]
testcase

Here's a test that you can run in Firebug or in the Inspector. It actually has turned up some bizarre behaviors in Firebug that we probably don't want to emulate (e.g., trying to format an object as an integer ends up printing "[Object object]"). I'll probably end up committing some variation of this in WebCore/manual-tests.

[Timothy Hatcher]

Comment on attachment 20571 [details]
Step 7: Return the unused substitutions from String.format

As talked about on IRC, returning a anonymous object with two named properties would be better.

[Adam Roben (:aroben)]

Created attachment 20592 [details]
Final step: Treat the first argument as a format string, concatenate additional arguments

[Adam Roben (:aroben)]

Comment on attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified

Committed in r31950

[Adam Roben (:aroben)]

Comment on attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Committed in r31951

[Adam Roben (:aroben)]

Comment on attachment 20561 [details]
Step 3 (v2): Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

Committed in r31952

[Adam Roben (:aroben)]

Comment on attachment 20566 [details]
Step 4: Separate out tokenizing of format strings

Committed in r31953

[Adam Roben (:aroben)]

Comment on attachment 20569 [details]
Step 5: Make the formatting functions used for each format specifier configurable

Committed in r31954

[Adam Roben (:aroben)]

Comment on attachment 20570 [details]
Step 6: Make String.format able to produce objects other than strings

Committed in r31955

[Adam Roben (:aroben)]

Comment on attachment 20571 [details]
Step 7: Return the unused substitutions from String.format

Committed in r31956

[Adam Roben (:aroben)]

Comment on attachment 20592 [details]
Final step: Treat the first argument as a format string, concatenate additional arguments

Committed in r31957

[Adam Roben (:aroben)]

Created attachment 20600 [details]
Fix a bug in the testcase

[Adam Roben (:aroben)]

Comment on attachment 20600 [details]
Fix a bug in the testcase

Committed in r31959