Changeset 182034 in webkit


Ignore:
Timestamp:
Mar 26, 2015, 4:12:39 PM (10 years ago)
Author:
ggaren@apple.com
Message:

Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site
​https://bugs.webkit.org/show_bug.cgi?id=142974

Reviewed by Joseph Pecoraro.

Source/JavaScriptCore:

This patch does two things:

(1) Restore JavaScriptCore's sanitization of line and column numbers to
one-based values.

We need this because WebCore sometimes provides huge negative column
numbers.

(2) Solve the attribute event listener line numbering problem a different
way: Rather than offseting all line numbers by -1 in an attribute event
listener in order to arrange for a custom result, instead use an explicit
feature for saying "all errors in this code should map to this line number".

  • bytecode/UnlinkedCodeBlock.cpp:

(JSC::UnlinkedFunctionExecutable::link):
(JSC::UnlinkedFunctionExecutable::fromGlobalCode):

  • bytecode/UnlinkedCodeBlock.h:
  • interpreter/Interpreter.cpp:

(JSC::StackFrame::computeLineAndColumn):
(JSC::GetStackTraceFunctor::operator()):

  • interpreter/Interpreter.h:
  • interpreter/StackVisitor.cpp:

(JSC::StackVisitor::Frame::computeLineAndColumn):

  • parser/ParserError.h:

(JSC::ParserError::toErrorObject): Plumb through an override line number.
When a function has an override line number, all syntax and runtime
errors in the function will map to it. This is useful for attribute event
listeners.

  • parser/SourceCode.h:

(JSC::SourceCode::SourceCode): Restore the old sanitization of line and
column numbers to one-based integers. It was kind of a hack to remove this.

  • runtime/Executable.cpp:

(JSC::ScriptExecutable::ScriptExecutable):
(JSC::FunctionExecutable::fromGlobalCode):

  • runtime/Executable.h:

(JSC::ScriptExecutable::setOverrideLineNo):
(JSC::ScriptExecutable::hasOverrideLineNo):
(JSC::ScriptExecutable::overrideLineNo):

  • runtime/FunctionConstructor.cpp:

(JSC::constructFunctionSkippingEvalEnabledCheck):

  • runtime/FunctionConstructor.h: Plumb through an override line number.

Source/WebCore:

  • bindings/js/JSLazyEventListener.cpp:

(WebCore::JSLazyEventListener::initializeJSFunction): Use the new override
line number API to guarantee that errors will map to the .html file locations
that we like.

  • bindings/js/ScriptController.cpp:

(WebCore::ScriptController::eventHandlerPosition): Added a FIXME to cover
some cases where our line and column numbers are still nonsense.

LayoutTests:

No test covering this ASSERT because I couldn't design a way to reproduce
it after trying for a few hours. Simply loading the original ASSERTing
content from disk is not enough to reproduce this bug.

  • fast/profiler/dead-time-expected.txt:
  • fast/profiler/inline-event-handler-expected.txt:
  • fast/profiler/stop-profiling-after-setTimeout-expected.txt: These are

progressions, where we used to get the line number wrong.

  • fast/dom/attribute-event-listener-errors-expected.txt: Added.
  • fast/dom/attribute-event-listener-errors.html: Added. This test covers

a subtle way in which the new mechanism for attribute event listener
line numbers is more accurate than the old one.

Location:
trunk
Files:
2 added
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r182016 r182034  
     12015-03-26  Geoffrey Garen  <ggaren@apple.com>
     2
     3        Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site
     4        https://bugs.webkit.org/show_bug.cgi?id=142974
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        No test covering this ASSERT because I couldn't design a way to reproduce
     9        it after trying for a few hours. Simply loading the original ASSERTing
     10        content from disk is not enough to reproduce this bug.
     11
     12        * fast/profiler/dead-time-expected.txt:
     13        * fast/profiler/inline-event-handler-expected.txt:
     14        * fast/profiler/stop-profiling-after-setTimeout-expected.txt: These are
     15        progressions, where we used to get the line number wrong.
     16
     17        * fast/dom/attribute-event-listener-errors-expected.txt: Added.
     18        * fast/dom/attribute-event-listener-errors.html: Added. This test covers
     19        a subtle way in which the new mechanism for attribute event listener
     20        line numbers is more accurate than the old one.
     21
    1222015-03-26  Brady Eidson  <beidson@apple.com>
    223

  • trunk/LayoutTests/fast/profiler/dead-time-expected.txt

    r181810 r182034  
    55Profile title: Dead time in profile.
    66Thread_1 (no file) (line 0:0)
    7    onload dead-time.html (line 20:52)
     7   onload dead-time.html (line 21:52)
    88      startTest dead-time.html (line 13:1)
    99         setTimeout (no file) (line 0:0)

  • trunk/LayoutTests/fast/profiler/inline-event-handler-expected.txt

    r181810 r182034  
    88      getElementById (no file) (line 0:0)
    99      click (no file) (line 0:0)
    10          onclick inline-event-handler.html (line 30:135)
     10         onclick inline-event-handler.html (line 31:135)
    1111            eventListener inline-event-handler.html (line 17:26)
    1212               anonymousFunction profiler-test-JS-resources.js (line 29:37)

  • trunk/LayoutTests/fast/profiler/stop-profiling-after-setTimeout-expected.txt

    r181810 r182034  
    55Profile title: Stop profiling from a timeout
    66Thread_1 (no file) (line 0:0)
    7    onload stop-profiling-after-setTimeout.html (line 20:52)
     7   onload stop-profiling-after-setTimeout.html (line 21:52)
    88      startTest stop-profiling-after-setTimeout.html (line 13:1)
    99         setTimeout (no file) (line 0:0)

  • trunk/Source/JavaScriptCore/ChangeLog

    r182023 r182034  
     12015-03-26  Geoffrey Garen  <ggaren@apple.com>
     2
     3        Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site
     4        https://bugs.webkit.org/show_bug.cgi?id=142974
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        This patch does two things:
     9
     10        (1) Restore JavaScriptCore's sanitization of line and column numbers to
     11        one-based values.
     12
     13        We need this because WebCore sometimes provides huge negative column
     14        numbers.
     15
     16        (2) Solve the attribute event listener line numbering problem a different
     17        way: Rather than offseting all line numbers by -1 in an attribute event
     18        listener in order to arrange for a custom result, instead use an explicit
     19        feature for saying "all errors in this code should map to this line number".
     20
     21        * bytecode/UnlinkedCodeBlock.cpp:
     22        (JSC::UnlinkedFunctionExecutable::link):
     23        (JSC::UnlinkedFunctionExecutable::fromGlobalCode):
     24        * bytecode/UnlinkedCodeBlock.h:
     25        * interpreter/Interpreter.cpp:
     26        (JSC::StackFrame::computeLineAndColumn):
     27        (JSC::GetStackTraceFunctor::operator()):
     28        * interpreter/Interpreter.h:
     29        * interpreter/StackVisitor.cpp:
     30        (JSC::StackVisitor::Frame::computeLineAndColumn):
     31        * parser/ParserError.h:
     32        (JSC::ParserError::toErrorObject): Plumb through an override line number.
     33        When a function has an override line number, all syntax and runtime
     34        errors in the function will map to it. This is useful for attribute event
     35        listeners.
     36 
     37        * parser/SourceCode.h:
     38        (JSC::SourceCode::SourceCode): Restore the old sanitization of line and
     39        column numbers to one-based integers. It was kind of a hack to remove this.
     40
     41        * runtime/Executable.cpp:
     42        (JSC::ScriptExecutable::ScriptExecutable):
     43        (JSC::FunctionExecutable::fromGlobalCode):
     44        * runtime/Executable.h:
     45        (JSC::ScriptExecutable::setOverrideLineNo):
     46        (JSC::ScriptExecutable::hasOverrideLineNo):
     47        (JSC::ScriptExecutable::overrideLineNo):
     48        * runtime/FunctionConstructor.cpp:
     49        (JSC::constructFunctionSkippingEvalEnabledCheck):
     50        * runtime/FunctionConstructor.h: Plumb through an override line number.
     51
    1522015-03-26  Filip Pizlo  <fpizlo@apple.com>
    253

  • trunk/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp

    r181993 r182034  
    133133}
    134134
    135 FunctionExecutable* UnlinkedFunctionExecutable::link(VM& vm, const SourceCode& ownerSource)
     135FunctionExecutable* UnlinkedFunctionExecutable::link(VM& vm, const SourceCode& ownerSource, int overrideLineNo)
    136136{
    137137    SourceCode source = m_sourceOverride ? SourceCode(m_sourceOverride) : ownerSource;
    … …  
    146146
    147147    SourceCode code(source.provider(), startOffset, startOffset + m_sourceLength, firstLine, startColumn);
    148     return FunctionExecutable::create(vm, code, this, firstLine, firstLine + m_lineCount, startColumn, endColumn);
    149 }
    150 
    151 UnlinkedFunctionExecutable* UnlinkedFunctionExecutable::fromGlobalCode(const Identifier& name, ExecState& exec, const SourceCode& source, JSObject*& exception)
     148    FunctionExecutable* result = FunctionExecutable::create(vm, code, this, firstLine, firstLine + m_lineCount, startColumn, endColumn);
     149    if (overrideLineNo != -1)
     150        result->setOverrideLineNo(overrideLineNo);
     151    return result;
     152}
     153
     154UnlinkedFunctionExecutable* UnlinkedFunctionExecutable::fromGlobalCode(
     155    const Identifier& name, ExecState& exec, const SourceCode& source,
     156    JSObject*& exception, int overrideLineNo)
    152157{
    153158    ParserError error;
    … …  
    161166
    162167    if (error.isValid()) {
    163         exception = error.toErrorObject(&globalObject, source);
     168        exception = error.toErrorObject(&globalObject, source, overrideLineNo);
    164169        return nullptr;
    165170    }

  • trunk/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h

    r181993 r182034  
    137137        ParserError&);
    138138
    139     static UnlinkedFunctionExecutable* fromGlobalCode(const Identifier&, ExecState&, const SourceCode&, JSObject*& exception);
    140 
    141     FunctionExecutable* link(VM&, const SourceCode&);
     139    static UnlinkedFunctionExecutable* fromGlobalCode(
     140        const Identifier&, ExecState&, const SourceCode&, JSObject*& exception,
     141        int overrideLineNo);
     142
     143    FunctionExecutable* link(VM&, const SourceCode&, int overrideLineNo = -1);
    142144
    143145    void clearCodeForRecompilation()

  • trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

    r181993 r182034  
    441441    line = divotLine + lineOffset;
    442442    column = divotColumn + (divotLine ? 1 : firstLineColumnOffset);
     443
     444    if (executable->hasOverrideLineNo())
     445        line = executable->overrideLineNo();
    443446}
    444447
    … …  
    491494                    Strong<JSObject>(vm, visitor->callee()),
    492495                    getStackFrameCodeType(visitor),
    493                     Strong<ExecutableBase>(vm, codeBlock->ownerExecutable()),
     496                    Strong<ScriptExecutable>(vm, codeBlock->ownerExecutable()),
    494497                    Strong<UnlinkedCodeBlock>(vm, codeBlock->unlinkedCodeBlock()),
    495498                    codeBlock->source(),
    … …  
    502505                m_results.append(s);
    503506            } else {
    504                 StackFrame s = { Strong<JSObject>(vm, visitor->callee()), StackFrameNativeCode, Strong<ExecutableBase>(), Strong<UnlinkedCodeBlock>(), 0, 0, 0, 0, 0, String()};
     507                StackFrame s = { Strong<JSObject>(vm, visitor->callee()), StackFrameNativeCode, Strong<ScriptExecutable>(), Strong<UnlinkedCodeBlock>(), 0, 0, 0, 0, 0, String()};
    505508                m_results.append(s);
    506509            }

  • trunk/Source/JavaScriptCore/interpreter/Interpreter.h

    r181993 r182034  
    8282        Strong<JSObject> callee;
    8383        StackFrameCodeType codeType;
    84         Strong<ExecutableBase> executable;
     84        Strong<ScriptExecutable> executable;
    8585        Strong<UnlinkedCodeBlock> codeBlock;
    8686        RefPtr<SourceProvider> code;

  • trunk/Source/JavaScriptCore/interpreter/StackVisitor.cpp

    r181993 r182034  
    294294    line = divotLine + codeBlock->ownerExecutable()->lineNo();
    295295    column = divotColumn + (divotLine ? 1 : codeBlock->firstLineColumnOffset());
     296
     297    if (codeBlock->ownerExecutable()->hasOverrideLineNo())
     298        line = codeBlock->ownerExecutable()->overrideLineNo();
    296299}
    297300

  • trunk/Source/JavaScriptCore/parser/ParserError.h

    r180637 r182034  
    8686    int line() const { return m_line; }
    8787
    88     JSObject* toErrorObject(JSGlobalObject* globalObject, const SourceCode& source)
     88    JSObject* toErrorObject(
     89        JSGlobalObject* globalObject, const SourceCode& source,
     90        int overrideLineNo = -1)
    8991    {
    9092        switch (m_type) {
    … …  
    9294            return nullptr;
    9395        case SyntaxError:
    94             return addErrorInfo(globalObject->globalExec(), createSyntaxError(globalObject, m_message), m_line, source);
     96            return addErrorInfo(
     97                globalObject->globalExec(),
     98                createSyntaxError(globalObject, m_message),
     99                overrideLineNo == -1 ? m_line : overrideLineNo, source);
    95100        case EvalError:
    96101            return createSyntaxError(globalObject, m_message);

  • trunk/Source/JavaScriptCore/parser/SourceCode.h

    r181810 r182034  
    6464            , m_startChar(0)
    6565            , m_endChar(m_provider->source().length())
    66             , m_firstLine(std::max(firstLine, 0))
    67             , m_startColumn(std::max(startColumn, 0))
     66            , m_firstLine(std::max(firstLine, 1))
     67            , m_startColumn(std::max(startColumn, 1))
    6868        {
    6969        }
    … …  
    7373            , m_startChar(start)
    7474            , m_endChar(end)
    75             , m_firstLine(std::max(firstLine, 0))
    76             , m_startColumn(std::max(startColumn, 0))
     75            , m_firstLine(std::max(firstLine, 1))
     76            , m_startColumn(std::max(startColumn, 1))
    7777        {
    7878        }

  • trunk/Source/JavaScriptCore/runtime/Executable.cpp

    r181901 r182034  
    101101    , m_neverInline(false)
    102102    , m_didTryToEnterInLoop(false)
     103    , m_overrideLineNo(-1)
    103104    , m_firstLine(-1)
    104105    , m_lastLine(-1)
    … …  
    609610}
    610611
    611 FunctionExecutable* FunctionExecutable::fromGlobalCode(const Identifier& name, ExecState& exec, const SourceCode& source, JSObject*& exception)
    612 {
    613     UnlinkedFunctionExecutable* unlinkedExecutable = UnlinkedFunctionExecutable::fromGlobalCode(name, exec, source, exception);
     612FunctionExecutable* FunctionExecutable::fromGlobalCode(
     613    const Identifier& name, ExecState& exec, const SourceCode& source,
     614    JSObject*& exception, int overrideLineNo)
     615{
     616    UnlinkedFunctionExecutable* unlinkedExecutable =
     617        UnlinkedFunctionExecutable::fromGlobalCode(
     618            name, exec, source, exception, overrideLineNo);
    614619    if (!unlinkedExecutable)
    615620        return nullptr;
    616     return unlinkedExecutable->link(exec.vm(), source);
     621
     622    return unlinkedExecutable->link(exec.vm(), source, overrideLineNo);
    617623}
    618624

  • trunk/Source/JavaScriptCore/runtime/Executable.h

    r181901 r182034  
    359359    const String& sourceURL() const { return m_source.provider()->url(); }
    360360    int lineNo() const { return m_firstLine; }
     361    void setOverrideLineNo(int overrideLineNo) { m_overrideLineNo = overrideLineNo; }
     362    bool hasOverrideLineNo() const { return m_overrideLineNo != -1; }
     363    int overrideLineNo() const { return m_overrideLineNo; }
    361364    int lastLine() const { return m_lastLine; }
    362365    unsigned startColumn() const { return m_startColumn; }
    … …  
    430433    bool m_neverInline;
    431434    bool m_didTryToEnterInLoop;
     435    int m_overrideLineNo;
    432436    int m_firstLine;
    433437    int m_lastLine;
    … …  
    550554        return executable;
    551555    }
    552     static FunctionExecutable* fromGlobalCode(const Identifier& name, ExecState&, const SourceCode&, JSObject*& exception);
     556    static FunctionExecutable* fromGlobalCode(
     557        const Identifier& name, ExecState&, const SourceCode&,
     558        JSObject*& exception, int overrideLineNo);
    553559
    554560    static void destroy(JSCell*);

  • trunk/Source/JavaScriptCore/runtime/FunctionConstructor.cpp

    r181810 r182034  
    8787}
    8888
    89 JSObject* constructFunctionSkippingEvalEnabledCheck(ExecState* exec, JSGlobalObject* globalObject, const ArgList& args, const Identifier& functionName, const String& sourceURL, const TextPosition& position)
     89JSObject* constructFunctionSkippingEvalEnabledCheck(
     90    ExecState* exec, JSGlobalObject* globalObject, const ArgList& args,
     91    const Identifier& functionName, const String& sourceURL,
     92    const TextPosition& position, int overrideLineNo)
    9093{
    9194    // How we stringify functions is sometimes important for web compatibility.
    … …  
    114117    SourceCode source = makeSource(program, sourceURL, position);
    115118    JSObject* exception = nullptr;
    116     FunctionExecutable* function = FunctionExecutable::fromGlobalCode(functionName, *exec, source, exception);
     119    FunctionExecutable* function = FunctionExecutable::fromGlobalCode(functionName, *exec, source, exception, overrideLineNo);
    117120    if (!function) {
    118121        ASSERT(exception);

  • trunk/Source/JavaScriptCore/runtime/FunctionConstructor.h

    r173269 r182034  
    6060JSObject* constructFunction(ExecState*, JSGlobalObject*, const ArgList&);
    6161
    62 JS_EXPORT_PRIVATE JSObject* constructFunctionSkippingEvalEnabledCheck(ExecState*, JSGlobalObject*, const ArgList&, const Identifier&, const String&, const WTF::TextPosition&);
     62JS_EXPORT_PRIVATE JSObject* constructFunctionSkippingEvalEnabledCheck(
     63    ExecState*, JSGlobalObject*, const ArgList&, const Identifier&,
     64    const String&, const WTF::TextPosition&, int overrideLineNo = -1);
    6365
    6466} // namespace JSC

  • trunk/Source/WebCore/ChangeLog

    r182033 r182034  
     12015-03-26  Geoffrey Garen  <ggaren@apple.com>
     2
     3        Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site
     4        https://bugs.webkit.org/show_bug.cgi?id=142974
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        * bindings/js/JSLazyEventListener.cpp:
     9        (WebCore::JSLazyEventListener::initializeJSFunction): Use the new override
     10        line number API to guarantee that errors will map to the .html file locations
     11        that we like.
     12
     13        * bindings/js/ScriptController.cpp:
     14        (WebCore::ScriptController::eventHandlerPosition): Added a FIXME to cover
     15        some cases where our line and column numbers are still nonsense.
     16
    1172015-03-26  Beth Dakin  <bdakin@apple.com>
    218

  • trunk/Source/WebCore/bindings/js/JSLazyEventListener.cpp

    r181810 r182034  
    2525#include "JSNode.h"
    2626#include "ScriptController.h"
     27#include <runtime/Executable.h>
    2728#include <runtime/FunctionConstructor.h>
    2829#include <runtime/IdentifierInlines.h>
    … …  
    104105    args.append(jsStringWithCache(exec, m_code));
    105106
    106     // Move our text position backward one line. Creating an anonymous function
    107     // will add a line for a function declaration, but we want our line number
    108     // to match up with where the attribute was declared.
    109     TextPosition position(
    110         OrdinalNumber::fromOneBasedInt(
    111             m_position.m_line.oneBasedInt() - 1), m_position.m_column);
     107    // We want all errors to refer back to the line on which our attribute was
     108    // declared, regardless of any newlines in our JavaScript source text.
     109    int overrideLineNo = m_position.m_line.oneBasedInt();
     110
    112111    JSObject* jsFunction = constructFunctionSkippingEvalEnabledCheck(
    113112        exec, exec->lexicalGlobalObject(), args, Identifier(exec, m_functionName),
    114         m_sourceURL, position);
     113        m_sourceURL, m_position, overrideLineNo);
    115114
    116115    if (exec->hadException()) {
    … …  
    121120
    122121    JSFunction* listenerAsFunction = jsCast<JSFunction*>(jsFunction);
     122
    123123    if (m_originalNode) {
    124124        if (!wrapper()) {

  • trunk/Source/WebCore/bindings/js/ScriptController.cpp

    r181925 r182034  
    275275TextPosition ScriptController::eventHandlerPosition() const
    276276{
     277    // FIXME: If we are not currently parsing, we should use our current location
     278    // in JavaScript, to cover cases like "element.setAttribute('click', ...)".
     279
     280    // FIXME: This location maps to the end of the HTML tag, and not to the
     281    // exact column number belonging to the event handler attribute.
    277282    ScriptableDocumentParser* parser = m_frame.document()->scriptableDocumentParser();
    278283    if (parser)
Note: See TracChangeset for help on using the changeset viewer.