Bug 119575 – Restoring use of StackIterator instead of Interpreter::getStacktrace()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This change set reverts r153383 (https://bugs.webkit.org/show_bug.cgi?id=119141) by adding a numberOfFrames() method to the StackIterator class.  This enables us to achieve the createScriptCallStack() semantics that the inspector expects.  This fixes the issue in https://bugs.webkit.org/show_bug.cgi?id=119141, and allows us to pass the "plugins" layout tests.  It is questionable whether it is appropriate for the inspector to expect those semantics, but that's a question for another day. 

This change set also makes Interpreter::getStackTrace() private again so that "unauthorized" code is not encouraged to call it.  This results in "moving" some ErrorConstructor and NativeErrorConstructor functions under the Interpreter.

[Mark Lam]

Created attachment 208347 [details]
the patch

[Oliver Hunt]

Comment on attachment 208347 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review

> Source/JavaScriptCore/ChangeLog:19
> +        (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame.

goto is a single word so the old capitalization was correct

> Source/JavaScriptCore/ChangeLog:32
> +        (JSC::ErrorConstructor::getCallData):
> +        - Don't want ErrorConstructor to call Interpreter::getStackTrace()
> +          directly. So, we moved the helper functions into the Interpreter
> +          class.

Haven't read the full patch yet, but if possible it would be nice to have changes that should not effect behavior moved into a separate bug, and landed earlier if possible

[Oliver Hunt]

Comment on attachment 208347 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review

> Source/JavaScriptCore/interpreter/Interpreter.h:229
> +        static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
> +        static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
> +        static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*);
> +        static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*);
> +

I like this, but it seems like this can be a separate patch, r+ for anding it separately

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 208347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review
> 
> > Source/JavaScriptCore/ChangeLog:19
> > +        (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame.
> 
> goto is a single word so the old capitalization was correct

Hmmm ... not in English AFAIK, but I'm ok with sticking with "goto" if there are no objection from anyone else.

(In reply to comment #3)
> (From update of attachment 208347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.h:229
> > +        static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
> > +        static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
> > +        static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*);
> > +        static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*);
> > +
> 
> I like this, but it seems like this can be a separate patch, r+ for anding it separately

Will land this separately.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 208347 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:19
> > > +        (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame.
> > 
> > goto is a single word so the old capitalization was correct
> 
> Hmmm ... not in English AFAIK, but I'm ok with sticking with "goto" if there are no objection from anyone else.

goto doesn't get flagged as wrong by spellcheck or autocorrect :D

> 
> (In reply to comment #3)
> > (From update of attachment 208347 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review
> > 
> > > Source/JavaScriptCore/interpreter/Interpreter.h:229
> > > +        static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
> > > +        static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
> > > +        static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*);
> > > +        static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*);
> > > +
> > 
> > I like this, but it seems like this can be a separate patch, r+ for anding it separately
> 
> Will land this separately.

[Mark Lam]

Created attachment 208353 [details]
updated patch: use "goto", and ErrorConstructor changes landed separately.

[Mark Lam]

Thanks for the review.  Revised patch landed in r153825: <http://trac.webkit.org/r153825>.