Bug 166348 – Propagate the source origin as much as possible

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Propagate the source URL as much as possible

[Yusuke Suzuki]

Created attachment 297602 [details]
Patch

It paves the way to implement dynamic-import

[Yusuke Suzuki]

Created attachment 297603 [details]
Patch

[Yusuke Suzuki]

Created attachment 297606 [details]
Patch

[WebKit Commit Bot]

This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)

[Yusuke Suzuki]

Created attachment 297609 [details]
Patch

[Alex Christensen]

Comment on attachment 297609 [details]
Patch

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

> Source/JavaScriptCore/parser/SourceCode.h:82
> +    inline SourceCode makeSource(const String& source, const String& url, const TextPosition& startPosition = TextPosition(), SourceProviderSourceType sourceType = SourceProviderSourceType::Program)

Are there many places in JavaScriptCore where we pass URLs around? A String and a URL shouldn't be completely interchangeable.  Should we move URL.h and URL.cpp to WTF?

[Build Bot]

Comment on attachment 297609 [details]
Patch

Attachment 297609 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2766212

New failing tests:
fast/events/window-onerror4.html
js/dom/stack-trace.html

[Build Bot]

Created attachment 297618 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Comment on attachment 297609 [details]
Patch

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

>> Source/JavaScriptCore/parser/SourceCode.h:82
>> +    inline SourceCode makeSource(const String& source, const String& url, const TextPosition& startPosition = TextPosition(), SourceProviderSourceType sourceType = SourceProviderSourceType::Program)
> 
> Are there many places in JavaScriptCore where we pass URLs around? A String and a URL shouldn't be completely interchangeable.  Should we move URL.h and URL.cpp to WTF?

Super unfortunate thing is that this `url` is not limited in URL.
For example, when JSC is executed in the jsc shell, this url is filled with UNIX (or Windows) file path.
We should rename it to more appropriate name at some point.

On the other hand, SourceProvider::m_sourceURLDirective, m_sourceURLMapping (it is not touched in this patch) should be std::optional<URL>.
They are related to source map's URL directive. I think that importing URL into WTF is worth doing. This should be done in the separate patch.

[Yusuke Suzuki]

Created attachment 297750 [details]
Patch

[Yusuke Suzuki]

Created attachment 297751 [details]
Patch

[Darin Adler]

Comment on attachment 297751 [details]
Patch

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

I like the idea of plumbing the source origin through everywhere, but I am unsure about the strategy of wrapping a string in a class just to give it the meaning "source origin".

> Source/JavaScriptCore/API/JSBase.cpp:65
> +    String url = sourceURL ? sourceURL->string() : String();

I would probably name this local sourceURLString.

> Source/JavaScriptCore/API/JSBase.cpp:66
> +    SourceCode source = makeSource(script->string(), SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));

Instead of SourceOrigin(url) could you write one of these?

    { sourceURLString }
    SourceOrigin { sourceURLString }

> Source/JavaScriptCore/API/JSBase.cpp:104
> +    String url = sourceURL ? sourceURL->string() : String();
> +    SourceCode source = makeSource(script->string(), SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));

Ditto.

> Source/JavaScriptCore/API/JSObjectRef.cpp:150
> +    auto url = sourceURL ? sourceURL->string() : String();
> +    JSObject* result = constructFunction(exec, exec->lexicalGlobalObject(), args, nameID, SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));

Ditto.

> Source/JavaScriptCore/API/JSScriptRef.cpp:44
> +    static WTF::RefPtr<OpaqueJSScript> create(VM* vm, const SourceOrigin& sourceOrigin, const String& url, int startingLineNumber, const String& source)

If we have to touch this anyway, then I suggest changing VM* to VM&.

> Source/JavaScriptCore/API/JSScriptRef.cpp:62
> +    OpaqueJSScript(VM* vm, const SourceOrigin& sourceOrigin, const String& url, int startingLineNumber, const String& source)

Ditto.

> Source/JavaScriptCore/API/JSScriptRef.cpp:97
> +    auto sourceURL = url ? url->string() : String();
> +    auto result = OpaqueJSScript::create(vm, SourceOrigin(sourceURL), sourceURL, startingLineNumber, String(StringImpl::createFromLiteral(source, length)));

Same comments as above.

> Source/JavaScriptCore/API/JSScriptRef.cpp:119
> +    auto sourceURL = url ? url->string() : String();
> +    auto result = OpaqueJSScript::create(vm, SourceOrigin(sourceURL), sourceURL, startingLineNumber, source->string());

Ditto.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:135
> +#define INITIALIZE_BUILTIN_SOURCE_MEMBERS(name, functionName, length) , m_##name##Source(JSC::makeSource(StringImpl::createFromLiteral(s_##name, length), JSC::SourceOrigin()))

Maybe { } instead of JSC::SourceOrigin()?

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:39
> +#define INITIALIZE_BUILTIN_SOURCE_MEMBERS(name, functionName, length) , m_##name##Source(makeSource(StringImpl::createFromLiteral(s_##name, length), SourceOrigin()))

Ditto.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:54
> +        return createExecutable(m_vm, makeSource(baseConstructorCode, SourceOrigin()), name, constructorKind, ConstructAbility::CanConstruct);

Ditto.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:56
> +        return createExecutable(m_vm, makeSource(derivedConstructorCode, SourceOrigin()), name, constructorKind, ConstructAbility::CanConstruct);

Ditto.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:246
> +    SourceOrigin sourceOrigin = callFrame->callerSourceOrigin();

Not sure this needs to be in a local variable. It’s only used once.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:247
> +    EvalExecutable* eval = DirectEvalExecutable::create(callFrame, makeSource(script, sourceOrigin), codeBlock->isStrictMode(), codeBlock->unlinkedCodeBlock()->derivedContextType(), codeBlock->unlinkedCodeBlock()->isArrowFunction(), evalContextType, &variablesUnderTDZ);

Sure seems like this should be auto* executable instead of EvalExcuteable* eval, but that’s not related to your patch I guess.

> Source/JavaScriptCore/inspector/InjectedScriptManager.cpp:141
> +    SourceCode sourceCode = makeSource(source, SourceOrigin());

I would prefer { }.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:111
> +    SourceOrigin sourceOrigin = exec->callerSourceOrigin();
> +    JSValue result = JSC::evaluateWithScopeExtension(exec, makeSource(program, sourceOrigin), scopeExtension, exception);

I suggest no local variable.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:92
> +    checkSyntax(m_vm, JSC::makeSource(expression, SourceOrigin()), error);

I think { }.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:223
> +    bool skipFirstFrame = false;

I think this should be named "haveSkippedFirstFrame".

> Source/JavaScriptCore/jsc.cpp:1107
> +    return makeSource(str, SourceOrigin(filename), filename);

I think { filename } or SourceOrigin { filename } would be better.

> Source/JavaScriptCore/jsc.cpp:1954
> +    SourceOrigin sourceOrigin = exec->callerSourceOrigin();

Better without local?

> Source/JavaScriptCore/jsc.cpp:1995
> +    SourceOrigin sourceOrigin = exec->callerSourceOrigin();

Better without local?

> Source/JavaScriptCore/jsc.cpp:2118
> +EncodedJSValue JSC_HOST_CALL functionCallerSourceOrigin(ExecState* exec)

How about "state" instead of "exec"?

> Source/JavaScriptCore/parser/SourceProvider.h:48
> +        JS_EXPORT_PRIVATE SourceProvider(const SourceOrigin&, const String& url, const TextPosition& startPosition, SourceProviderSourceType);

Do we really need both the SourceOrigin and the URL? Can we change things so we don’t?

> Source/JavaScriptCore/runtime/SourceOrigin.h:46
> +class SourceOrigin {
> +public:
> +    explicit SourceOrigin(const String& string)
> +        : m_string(string)
> +    {
> +    }
> +
> +    SourceOrigin() = default;
> +
> +    const String& string() const { return m_string; }
> +    bool isNull() const { return m_string.isNull(); }
> +
> +private:
> +    String m_string;
> +};

Seems overkill to wrap a String in a class just to give it the SourceOrigin semantic. Is there some cleaner way to do this?

> Source/WebCore/bindings/js/ScriptSourceCode.h:46
> +        : m_provider(JSC::StringSourceProvider::create(source, JSC::SourceOrigin(url.isNull() ? String() : url.string()), url.isNull() ? String() : url.string(), startPosition, sourceType))

There is no need for all this isNull() checking. A null URL returns a null string when URL::string() is called, so both calls to isNull on this line of code are unneeded.

[Yusuke Suzuki]

Comment on attachment 297751 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/API/JSBase.cpp:65
>> +    String url = sourceURL ? sourceURL->string() : String();
> 
> I would probably name this local sourceURLString.

Nice. Fixed.

>> Source/JavaScriptCore/API/JSBase.cpp:66
>> +    SourceCode source = makeSource(script->string(), SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));
> 
> Instead of SourceOrigin(url) could you write one of these?
> 
>     { sourceURLString }
>     SourceOrigin { sourceURLString }

Yeah, we can do that. I choosed `SourceOrigin { sourceURLString }`. Fixed.

>> Source/JavaScriptCore/API/JSBase.cpp:104
>> +    SourceCode source = makeSource(script->string(), SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/API/JSObjectRef.cpp:150
>> +    JSObject* result = constructFunction(exec, exec->lexicalGlobalObject(), args, nameID, SourceOrigin(url), url, TextPosition(OrdinalNumber::fromOneBasedInt(startingLineNumber), OrdinalNumber()));
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/API/JSScriptRef.cpp:44
>> +    static WTF::RefPtr<OpaqueJSScript> create(VM* vm, const SourceOrigin& sourceOrigin, const String& url, int startingLineNumber, const String& source)
> 
> If we have to touch this anyway, then I suggest changing VM* to VM&.

Yeah, it sounds nice. Fixed.

>> Source/JavaScriptCore/API/JSScriptRef.cpp:62
>> +    OpaqueJSScript(VM* vm, const SourceOrigin& sourceOrigin, const String& url, int startingLineNumber, const String& source)
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/API/JSScriptRef.cpp:97
>> +    auto result = OpaqueJSScript::create(vm, SourceOrigin(sourceURL), sourceURL, startingLineNumber, String(StringImpl::createFromLiteral(source, length)));
> 
> Same comments as above.

Fixed.

>> Source/JavaScriptCore/API/JSScriptRef.cpp:119
>> +    auto result = OpaqueJSScript::create(vm, SourceOrigin(sourceURL), sourceURL, startingLineNumber, source->string());
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:135
>> +#define INITIALIZE_BUILTIN_SOURCE_MEMBERS(name, functionName, length) , m_##name##Source(JSC::makeSource(StringImpl::createFromLiteral(s_##name, length), JSC::SourceOrigin()))
> 
> Maybe { } instead of JSC::SourceOrigin()?

Yeah, we can do that. Fixed.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:39
>> +#define INITIALIZE_BUILTIN_SOURCE_MEMBERS(name, functionName, length) , m_##name##Source(makeSource(StringImpl::createFromLiteral(s_##name, length), SourceOrigin()))
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:54
>> +        return createExecutable(m_vm, makeSource(baseConstructorCode, SourceOrigin()), name, constructorKind, ConstructAbility::CanConstruct);
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:56
>> +        return createExecutable(m_vm, makeSource(derivedConstructorCode, SourceOrigin()), name, constructorKind, ConstructAbility::CanConstruct);
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:246
>> +    SourceOrigin sourceOrigin = callFrame->callerSourceOrigin();
> 
> Not sure this needs to be in a local variable. It’s only used once.

We can drop this local variable. Dropped.

>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:247
>> +    EvalExecutable* eval = DirectEvalExecutable::create(callFrame, makeSource(script, sourceOrigin), codeBlock->isStrictMode(), codeBlock->unlinkedCodeBlock()->derivedContextType(), codeBlock->unlinkedCodeBlock()->isArrowFunction(), evalContextType, &variablesUnderTDZ);
> 
> Sure seems like this should be auto* executable instead of EvalExcuteable* eval, but that’s not related to your patch I guess.

Yeah, fixed.

>> Source/JavaScriptCore/inspector/InjectedScriptManager.cpp:141
>> +    SourceCode sourceCode = makeSource(source, SourceOrigin());
> 
> I would prefer { }.

Fixed.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:111
>> +    JSValue result = JSC::evaluateWithScopeExtension(exec, makeSource(program, sourceOrigin), scopeExtension, exception);
> 
> I suggest no local variable.

Dropped.

>> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:92
>> +    checkSyntax(m_vm, JSC::makeSource(expression, SourceOrigin()), error);
> 
> I think { }.

Fixed.

>> Source/JavaScriptCore/interpreter/CallFrame.cpp:223
>> +    bool skipFirstFrame = false;
> 
> I think this should be named "haveSkippedFirstFrame".

Sounds nice. Fixed.

>> Source/JavaScriptCore/jsc.cpp:1107
>> +    return makeSource(str, SourceOrigin(filename), filename);
> 
> I think { filename } or SourceOrigin { filename } would be better.

Fixed.

>> Source/JavaScriptCore/jsc.cpp:1954
>> +    SourceOrigin sourceOrigin = exec->callerSourceOrigin();
> 
> Better without local?

Dropped.

>> Source/JavaScriptCore/jsc.cpp:1995
>> +    SourceOrigin sourceOrigin = exec->callerSourceOrigin();
> 
> Better without local?

Dropped.

>> Source/JavaScriptCore/jsc.cpp:2118
>> +EncodedJSValue JSC_HOST_CALL functionCallerSourceOrigin(ExecState* exec)
> 
> How about "state" instead of "exec"?

I think keeping `exec` here is better for now since the other code uses ExecState*.

Actually, `ExecState*` is named in the various forms.
For example, this `ExecState*` is typedefed as `CallFrame*`. In that place, we use `CallFrame* callFrame`.
Maybe, before renaming bulk of these things, I think we need some agreement.

>> Source/JavaScriptCore/parser/SourceProvider.h:48
>> +        JS_EXPORT_PRIVATE SourceProvider(const SourceOrigin&, const String& url, const TextPosition& startPosition, SourceProviderSourceType);
> 
> Do we really need both the SourceOrigin and the URL? Can we change things so we don’t?

Consider the following case, using dynamic-import (it will be implemented in JSC) in eval.

`eval("import(...)")`.

In that case, SourceOrigin will become the caller's SourceOrigin (it will be used for the base of the dynamic import).
However, source URL should be null I think. If we use this SourceOrigin as the source URL,
when some error occurs, we will dump the stack trace like,

source-origin-url:line:column

But it is misleading. This error does not happen in source-origin-url:line:column place.
And the above thing can be applied to the function code if we create the function with the form like `new Function("import(...)")`.

So, in this patch, I separate SourceOrigin from the source URL.

>> Source/JavaScriptCore/runtime/SourceOrigin.h:46
>> +};
> 
> Seems overkill to wrap a String in a class just to give it the SourceOrigin semantic. Is there some cleaner way to do this?

Currently, we just store the String. But it may have more information I think.
For example, it may need to hold the information for module loader context (or may not).

So I think creating SourceOrigin class is safer & cleaner. (It also applies strict type check between SourceOrigin and source URL (string)).

[Yusuke Suzuki]

Created attachment 297761 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r210149: <http://trac.webkit.org/changeset/210149>