Streamline Console.cpp, abstract out the use of JSC::ExecState and JSC::ArgList by introducing ScriptCallFrame and ScriptStackTrace abstractions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Eliminate the need for custom bindings in Console.cpp and make argument and stack-trace handling more JS engine-agnostic by introducing two abstractions:

* ScriptCallFrame, which represents current JS call frame and provides access to call arguments and stack trace.
* ScriptStackFrame, which represents a frame in the stack trace.

This is also a good opportunity to give Console.cpp a few tweaks to make it more homogeneous.

[Dimitri Glazkov (Google)]

Created attachment 25713 [details]
ScriptCallFrame and ScriptStackTrace v1

Ok, this one is big. My apologies -- I tried to start small, but it was kind of hard to make meaningful changes without touching more files.

This patch also addresses:
* https://bugs.webkit.org/show_bug.cgi?id=22597
* https://bugs.webkit.org/show_bug.cgi?id=19136
* https://bugs.webkit.org/show_bug.cgi?id=21221

And makes some progress on:
* https://bugs.webkit.org/show_bug.cgi?id=21180

[Dimitri Glazkov (Google)]

Created attachment 25714 [details]
ScriptCallFrame and ScriptStackTrace v1

[Dimitri Glazkov (Google)]

I uploaded a wrong patch with https://bugs.webkit.org/show_bug.cgi?id=22631#c1, so please apply the contents of that comment to the patch below:

https://bugs.webkit.org/attachment.cgi?id=25714&action=prettypatch

[Timothy Hatcher]

Comment on attachment 25714 [details]
ScriptCallFrame and ScriptStackTrace v1

This patch looks good. Though I think the ScriptCallFrame and ScriptStackFrame classes are not clear.

I think more needs to be moved to ScriptStackFrame (like arguments, lineNumber and sourceURL). Then call ScriptCallFrame ScriptCallStack, and make it only return ScriptStackFrames. That would be a better design, and give more functionality.

I like the CustomArgumentHandling changes and the Firebug compat changes/comments!

r- based on the ScriptCallFrame and ScriptStackFrame classes design.

[Dimitri Glazkov (Google)]

Cool! I am glad you like it. What do you think about this idea?

* Move lineNumber, functionName, sourceURL to ScriptStackFrame
* Keep argument and frame accessors on ScriptCallFrame

This way, the stack trace is cleanly represented as a collection of ScriptCallFrames and ScriptCallFrame is essentially a way to access arguments and/or stack trace of the current call.

I don't see why we need to move arguments to ScriptStackFrame... unless you're thinking about eventually displaying the arguments for each frame?

As an aside note, the first frame in stack trace will have to be populated differently that the others, because currently there isn't a way to get line numbers, etc. from the JSC -- I think?

[Dimitri Glazkov (Google)]

Created attachment 25825 [details]
ScriptStackTrace and ScriptCallFrame v2

[Dimitri Glazkov (Google)]

Comment on attachment 25825 [details]
ScriptStackTrace and ScriptCallFrame v2

I reworked this a little bit to change the relationship and move methods around as follows:

* Now, the umbrella instance is ScriptStackTrace, which just has methods to access ScriptCallFrames and ScriptState

* argumentAt, argumentCount, functionName, lineNumber, etc. are now all at ScriptCallFrame.

* first ScriptCallFrame is created differently, because it is the only frame that has line number and arguments information at the moment.

Thanks for suggesting this, Timothy. This does look a bit cleaner.

[Dimitri Glazkov (Google)]

Created attachment 25846 [details]
ScriptCallStack and ScriptCallFrame v3

Darin (Fisher) suggested renaming ScriptStackTrace to ScriptCallStack. It seemed prettier, so I went ahead and did just that.

[Timothy Hatcher]

ScriptCallStack was also what I originally suggested. I will review this patch after lunch.

[Darin Adler]

Comment on attachment 25846 [details]
ScriptCallStack and ScriptCallFrame v3

> +    bindings/js/ScriptCalFrame.cpp \

Mispelling here.

> +ScriptCallFrame::ScriptCallFrame(const JSC::UString& functionName,
> +                                 const JSC::UString& urlString,
> +                                 int lineNumber,
> +                                 JSC::ExecState* exec,
> +                                 const JSC::ArgList& args,
> +                                 unsigned skipArgumentCount)

You should not need all those JSC prefixes here.

> +    for(unsigned i = skipArgumentCount; i < args.size(); ++i)

We normally call size() outside the loop. We normally use a space between "for" and "(".

> +const ScriptValue &ScriptCallFrame::argumentAt(unsigned index) const

We put the & next to the type name. ScriptValue&.

> +#include "runtime/ArgList.h"

This include should have angle brackets, I believe. We may also need a new forwarding header.

> +        const ScriptString &functionName() const { return m_functionName; }
> +        const KURL &sourceURL() const { return m_sourceURL; }

We put the & next to the type name. ScriptString& and KURL&.

> +        const ScriptValue &argumentAt(unsigned) const;

Ditto.

> +ScriptCallStack::ScriptCallStack(ExecState* exec, const ArgList& args,
> +                                   unsigned skipArgumentCount)

Strange indentation here. We normally try to avoid lining up second lines with parentheses for just this reason. Things don't line up any more if you rename. Instead we put things on one line or indent the second line by some fixed amount instead of lining up.

> +    JSValue* func;

Maybe "function" instead of "func".

> +    exec->interpreter()->retrieveLastCaller(exec, signedLineNumber, sourceID,
> +                                            urlString, func);

Another case if the "lining up a second line with parentheses".

> +        unsigned lineNumber =  signedLineNumber >= 0 ? signedLineNumber : 0;

Extra space here after the = sign.

> +        m_frames.append(
> +            ScriptCallFrame(m_caller->name(&m_exec->globalData()), urlString,
> +                            lineNumber, exec, args, skipArgumentCount));

Not a big fan of the line break before the ScriptCallFrame. I think that trying to line up subsequent lines with the open parenthesis leads to that decision because you want to preserve space for the second line. If the second line is just indented by 4 spaces I think that works better.

> +        // caller is unknown, but we should still add the frame, because
> +        // something called us, and gave us arguments

We almost always use sentence capitalization and periods for comments like this one.

> +    JSValue* func = m_exec->interpreter()->retrieveCaller(m_exec, m_caller);
> +    while(!func->isNull()) {

Space after while here before parenthesis here.

> +        InternalFunction* internalFunction = asInternalFunction(func);
> +        ArgList emptyArgList;
> +        m_frames.append(
> +            ScriptCallFrame(
> +                internalFunction->name(&m_exec->globalData()), UString(), 0, 0,
> +                emptyArgList, 0));

Same comments about indentation.

> +    if (!m_value.get())
> +        return false;

This get() here should not be necessary. I'm a little concerned that when m_value is 0 we return false from both isNull() and isUndefined(). What is the name for that state when m_value is 0? Why do we allow that state at all?

I notice that you're not using isUndefined() yet. We may find later that isNullOrUndefined() is more common than isUndefined().

> -    ConsoleMessage(MessageSource s, MessageLevel l, ExecState* exec, const ArgList& args, unsigned li, const String& u, unsigned g)
> +    ConsoleMessage(MessageSource s, MessageLevel l, ScriptCallStack* callStack, unsigned g, bool storeTrace = false)

Maybe const ScriptCallStack& would be a better type than ScriptCallStack*. Is there a reason to prefer a pointer here?

> +        if (storeTrace)
> +            for(unsigned i = 0; i < callStack->size(); ++i)
> +                frames[i] = callStack->at(i).functionName();

Our coding style puts braces around a multi-line if statement like this one. Also a space after the for before the parenthesis.

I believe that assigning to a UString requires holding the JSLock, so this new code should be added after the JSLock line. But maybe my information about that is out of date. It could date back to an earlier stage of our threading model.

> -void InspectorController::addMessageToConsole(MessageSource source, MessageLevel level, ExecState* exec, const ArgList& arguments, unsigned lineNumber, const String& sourceURL)
> +void InspectorController::addMessageToConsole(MessageSource source, MessageLevel level, ScriptCallStack* callStack)

Again, I think a const ScriptCallStack& would be better here.

> -void InspectorController::startGroup(MessageSource source, ExecState* exec, const ArgList& arguments, unsigned lineNumber, const String& sourceURL)
> +void InspectorController::startGroup(MessageSource source, ScriptCallStack* callStack)

Ditto.

>  #include "PlatformString.h"
> +#if USE(JSC)
>  #include <profiler/Profile.h>
> +#endif
>  #include <wtf/RefCounted.h>
>  #include <wtf/PassRefPtr.h>

Includes inside an #if should be in a separate paragraph.

My review comments are mostly minor things, but there are enough that I think I'll say review- for now. Seems like you're on the right track, though. Looking forward to the next patch. Tim, sorry I "grabbed this one out from under you".

[Dimitri Glazkov (Google)]

Created attachment 25871 [details]
ScriptCallStack and ScriptCallFrame v4

(In reply to comment #10)

Thanks for reviewing!

> (From update of attachment 25846 [details] [review])
> > +    bindings/js/ScriptCalFrame.cpp \
> 
> Mispelling here.

Fixed.

> > +ScriptCallFrame::ScriptCallFrame(const JSC::UString& functionName,
> > +                                 const JSC::UString& urlString,
> > +                                 int lineNumber,
> > +                                 JSC::ExecState* exec,
> > +                                 const JSC::ArgList& args,
> > +                                 unsigned skipArgumentCount)
> 
> You should not need all those JSC prefixes here.
> 
> > +    for(unsigned i = skipArgumentCount; i < args.size(); ++i)
> 
> We normally call size() outside the loop. We normally use a space between "for"
> and "(".

Fixed.

> > +const ScriptValue &ScriptCallFrame::argumentAt(unsigned index) const
> 
> We put the & next to the type name. ScriptValue&.
> 
> > +#include "runtime/ArgList.h"
> 
> This include should have angle brackets, I believe. We may also need a new
> forwarding header.

Fixed.

> > +        const ScriptString &functionName() const { return m_functionName; }
> > +        const KURL &sourceURL() const { return m_sourceURL; }
> 
> We put the & next to the type name. ScriptString& and KURL&.

Fixed.

> 
> > +        const ScriptValue &argumentAt(unsigned) const;
> 
> Ditto.

Fixed.

> 
> > +ScriptCallStack::ScriptCallStack(ExecState* exec, const ArgList& args,
> > +                                   unsigned skipArgumentCount)
> 
> Strange indentation here. We normally try to avoid lining up second lines with
> parentheses for just this reason. Things don't line up any more if you rename.
> Instead we put things on one line or indent the second line by some fixed
> amount instead of lining up.

I went ahead and made all of my changes run over 80 cols instead of lining up on next line.

> > +    JSValue* func;
> 
> Maybe "function" instead of "func".

Renamed to "function".

> 
> > +    exec->interpreter()->retrieveLastCaller(exec, signedLineNumber, sourceID,
> > +                                            urlString, func);
> 
> Another case if the "lining up a second line with parentheses".

Fixed.

> 
> > +        unsigned lineNumber =  signedLineNumber >= 0 ? signedLineNumber : 0;
> 
> Extra space here after the = sign.

Fixed.

> 
> > +        m_frames.append(
> > +            ScriptCallFrame(m_caller->name(&m_exec->globalData()), urlString,
> > +                            lineNumber, exec, args, skipArgumentCount));
> 
> Not a big fan of the line break before the ScriptCallFrame. I think that trying
> to line up subsequent lines with the open parenthesis leads to that decision
> because you want to preserve space for the second line. If the second line is
> just indented by 4 spaces I think that works better.

Fixed.

> 
> > +        // caller is unknown, but we should still add the frame, because
> > +        // something called us, and gave us arguments
> 
> We almost always use sentence capitalization and periods for comments like this
> one.

Fixed.

> > +    JSValue* func = m_exec->interpreter()->retrieveCaller(m_exec, m_caller);
> > +    while(!func->isNull()) {
> 
> Space after while here before parenthesis here.

Fixed.

> > +        InternalFunction* internalFunction = asInternalFunction(func);
> > +        ArgList emptyArgList;
> > +        m_frames.append(
> > +            ScriptCallFrame(
> > +                internalFunction->name(&m_exec->globalData()), UString(), 0, 0,
> > +                emptyArgList, 0));
> 
> Same comments about indentation.

Fixed.

> > +    if (!m_value.get())
> > +        return false;
> 
> This get() here should not be necessary. I'm a little concerned that when
> m_value is 0 we return false from both isNull() and isUndefined(). What is the
> name for that state when m_value is 0? Why do we allow that state at all?

Removed ".get()" from all methods. You're right, there's an overload that takes care of this.

As for the state, I would think that if someone writes "ProtectedPtr<JSValue> emptyValue;", they don't necessarily mean that the held value is null or undefined -- just that there isn't one being held by the wrapper. If we want to change the meaning of this, we should do it by changing ProtectedPtr by always populating with some default T constructor, which contains initial value. Right? I am not really sure, just thinking outloud.

> I notice that you're not using isUndefined() yet. We may find later that
> isNullOrUndefined() is more common than isUndefined().

I am using both isNull() and isUndefined() in a composite statement that's the equivalent of isNullOrUndefined(). I thought keeping them separate would be a good thing, since one can check for either together or separately, but I am open to just smushing them together into one method.

> 
> > -    ConsoleMessage(MessageSource s, MessageLevel l, ExecState* exec, const ArgList& args, unsigned li, const String& u, unsigned g)
> > +    ConsoleMessage(MessageSource s, MessageLevel l, ScriptCallStack* callStack, unsigned g, bool storeTrace = false)
> 
> Maybe const ScriptCallStack& would be a better type than ScriptCallStack*. Is
> there a reason to prefer a pointer here?

The reason I use a pointer is because ScriptCallFrame vector is lazily populated when first queried for stack trace, which contradicts the "constness" qualifier ... Oops, I just realized I have a problem where the laziness is not really lazy -- access to 0'th call frame should not trigger initialization. Fixed that.

> > +        if (storeTrace)
> > +            for(unsigned i = 0; i < callStack->size(); ++i)
> > +                frames[i] = callStack->at(i).functionName();
> 
> Our coding style puts braces around a multi-line if statement like this one.
> Also a space after the for before the parenthesis.

Fixed.

> I believe that assigning to a UString requires holding the JSLock, so this new
> code should be added after the JSLock line. But maybe my information about that
> is out of date. It could date back to an earlier stage of our threading model.

Sounds good. Addressed by using ScriptString, which does the locking for you.

> > -void InspectorController::addMessageToConsole(MessageSource source, MessageLevel level, ExecState* exec, const ArgList& arguments, unsigned lineNumber, const String& sourceURL)
> > +void InspectorController::addMessageToConsole(MessageSource source, MessageLevel level, ScriptCallStack* callStack)
> 
> Again, I think a const ScriptCallStack& would be better here.
> 
> > -void InspectorController::startGroup(MessageSource source, ExecState* exec, const ArgList& arguments, unsigned lineNumber, const String& sourceURL)
> > +void InspectorController::startGroup(MessageSource source, ScriptCallStack* callStack)
> 
> Ditto.
> 
> >  #include "PlatformString.h"
> > +#if USE(JSC)
> >  #include <profiler/Profile.h>
> > +#endif
> >  #include <wtf/RefCounted.h>
> >  #include <wtf/PassRefPtr.h>
> 
> Includes inside an #if should be in a separate paragraph.
> 
> My review comments are mostly minor things, but there are enough that I think
> I'll say review- for now. Seems like you're on the right track, though. Looking
> forward to the next patch. Tim, sorry I "grabbed this one out from under you".
> 

Thanks again for reviewing! This really helps me get up to speed on conventions and general WebKit way of life :)

Additionally, I started testing .trace behavior some more and realized that with the introduction of a separate frames vector, the comparison of messages needs a tweak. Added that in v4.

I've been thinking: it is kind of unfortunate that there's all this pouring of data from one fairly similar object (ScriptCallStack) to another (ConsoleMessage). Perhaps there needs to be some sort of safe, "frozen-in-time" ScriptCallStack, which ConsoleMessage can hold and pass around.

What do you think?

[Timothy Hatcher]

(In reply to comment #11)

> I've been thinking: it is kind of unfortunate that there's all this pouring of
> data from one fairly similar object (ScriptCallStack) to another
> (ConsoleMessage). Perhaps there needs to be some sort of safe, "frozen-in-time"
> ScriptCallStack, which ConsoleMessage can hold and pass around.

I think that sounds good.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/39142

[Darin Fisher (:fishd, Google)]

and the newly added files (doh!):
http://trac.webkit.org/changeset/39143

[Darin Fisher (:fishd, Google)]

fix windows bustage:
http://trac.webkit.org/changeset/39147