Bug 22721 – Implement WorkerUtils.importScripts()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexey Proskuryakov]

This is a way to include external code in scripts running in Workers: <http://www.whatwg.org/specs/web-workers/current-work/#importscripts>.

Much of the groundwork necessary for this is common with XMLHttpRequest.

[Alexey Proskuryakov]

<rdar://problem/6425807>

[Oliver Hunt]

Created attachment 28358 [details]
Initial pass at importScripts

Patcheration for fun and profit

[Alexey Proskuryakov]

Comment on attachment 28358 [details]
Initial pass at importScripts

I'm getting an assertion failure on layout tests that is caused by this patch: in WorkerScriptController::evaluate(), reportException() is no longer protected with JSLock. This is harmless in release builds, though.

I'm not sure if splitting evaluate() in two functions is necessary or helpful - can you just check exec->hadException() after each script?

-    if (comp.complType() == Throw)
-        reportException(exec, comp.value());

You've removed complType() check - was that intentional?

+    operator bool() { return m_value; }

That's one very dangerous operator, can you avoid adding it?

+#include "ScriptSourceCode.h"
+#include "WorkerImportScriptsClient.h"
+#include "ScriptValue.h"

Keep them sorted? I guess that's result of a global rename.

+        if (!m_responseEncoding.isEmpty())
+            m_decoder = TextResourceDecoder::create("text/javascript", m_responseEncoding);
+        else
+            m_decoder = TextResourceDecoder::create("text/javascript", "UTF-8");

This should really be document->encoding(), not "UTF-8". That depends on bug 24150, which is in commit queue at the moment.

+    m_failed = true;

Tell the Web Inspector?

+        WorkerImportScriptsClient(ScriptExecutionContext* scriptExecutionContext, const String& url, const String& callerURL, int callerLineNumber)

As you are well aware, there are multiple instances in WebCore where we lack script URL and line info to properly return errors - unconditionally passing them around does not scale. Surely, solving this old problem is out of scope for this bug.

+        ScriptString m_script;

Do we really need to use ScriptString here? For XHR, it was needed because responses are sometimes huge - certainly it's not the case for importScripts().

I've got enough comments to say r-, but this looks extremely good for an initial pass!

[Dmitry Titov]

> +        if (!m_responseEncoding.isEmpty())
> +            m_decoder = TextResourceDecoder::create("text/javascript",
> m_responseEncoding);
> +        else
> +            m_decoder = TextResourceDecoder::create("text/javascript",
> "UTF-8");
> 
> This should really be document->encoding(), not "UTF-8". That depends on bug
> 24150, which is in commit queue at the moment.

bug 24150 is landed, and m_executionContext->encoding() would do the right thing here. It contains the encoding of the original document.

[Oliver Hunt]

Created attachment 28382 [details]
Updated patch

[Oliver Hunt]

Second patch removes the same origin check on scripts as the spec has had those restrictions removed

[Oliver Hunt]

Created attachment 28383 [details]
Updated patch #2

Updated to ToT so i could use ScriptExecutionContext::encoding()

[Oliver Hunt]

Comment on attachment 28383 [details]
Updated patch #2

per whatwg ml discussion i have to update this for behaviour to match spec rather than firefox

[Oliver Hunt]

Created attachment 28393 [details]
Updated to match firefox behaviour, as well as a little tidy up

Here we go

[Alexey Proskuryakov]

Comment on attachment 28393 [details]
Updated to match firefox behaviour, as well as a little tidy up

> +        if (exec->hadException())
> +            return jsNull();

Why return jsNull in exceptional case, but jsUndefined in normal one?

> +    if (exception.jsValue())
> +        reportException(m_workerContextWrapper->globalExec(), exception.jsValue());

This reportException() call still isn't explicitly protected with JSLock. Does anything guarantee that a lock will be held?

+        bool executionForbidden() const { return m_executionForbidden; }
     private:

This is not a nice function to add to public interface: since the flag is set from a different thread, you need to lock WorkerScriptController::m_sharedDataMutex in order for its value to be propagated to the current processor, and it's possible that the result will be stale by the time this lock is released. Is it really necessary to check this flag where you do? I'm sure that the check before calling evaluate() can be removed, because this function does it internally, and I think that the loader will also handle his situation gracefully.

Also, we usually leave a blank line before "private".

+#include "config.h"
+
+#include "WorkerImportScriptsClient.h"
+
+#include "ScriptExecutionContext.h"

There shouldn't be a blank line after config.h.

+void WorkerImportScriptsClient::didReceiveResponse(const ResourceResponse& response)

So, importScripts has the same security model as <script>, right? Should we add a test to ensure that some future XHR refactoring doesn't accidentally enforce CORS restrictions on importScripts()?

Looks like the WHATWG discussion is still ongoing, so I hope that you won't mind me being nasty, and saying r- again. My main concerns are adding executionForbidden(), and missing JSLock.

[Alexey Proskuryakov]

Oh, and it looks like WorkerImportScriptsClient needs ENABLE(WORKERS) guards.

[Alexey Proskuryakov]

+    Vector<KURL> completedUrls;

And it's URLs, not Urls.

[Oliver Hunt]

Created attachment 28425 [details]
Implement importScripts, matching spec behaviour (which appears to be the consensus)

This updates the patch to match spec behaviour again (this appears to be the consensus from the whatwg list), and hopefully i've addressed all of alexey's comments

[Oliver Hunt]

In response to comments from dave_levin, i have removed the m_response field from WorkerImportScriptsClient as it was unnecessary

[Alexey Proskuryakov]

Comment on attachment 28425 [details]
Implement importScripts, matching spec behaviour (which appears to be the consensus)

You can test cross-origin loading better by loading one of the scripts from http://localhost:8000 (as http tests are loaded from http://127.0.0.1:8000).

Please add WorkerImportScriptsClient to non-Mac projects.

Still no JSLock in evaluate()? Per IRC discussion, it's just an unsaved file.

         virtual void resourceRetrievedByXMLHttpRequest(unsigned long identifier, const ScriptString& sourceString) = 0;
-
+        virtual void scriptImported(unsigned long, const String&) { ASSERT_NOT_REACHED(); }

Why is scriptImported() not a pure virtual function, like the above counterpart?

+        virtual void didFinishLoading(unsigned long /* identifier */);

No need to comment out the argument name in declaration.

> In response to comments from dave_levin, i have removed the m_response field
> from WorkerImportScriptsClient as it was unnecessary

A good point, it's unnecessary - but I still see it in the patch.

r=me, please consider the comments, especially fixing non-Mac builds.

[Oliver Hunt]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/workers/resources/worker-importScripts-differentOrigin.js
	A	LayoutTests/http/tests/workers/resources/worker-importScripts-source1.js
	A	LayoutTests/http/tests/workers/resources/worker-importScripts-source2.js
	A	LayoutTests/http/tests/workers/resources/worker-importScripts-syntaxError.js
	A	LayoutTests/http/tests/workers/resources/worker-importScripts.js
	A	LayoutTests/http/tests/workers/worker-importScripts-expected.txt
	A	LayoutTests/http/tests/workers/worker-importScripts.html
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSWorkerContextCustom.cpp
	M	WebCore/bindings/js/ScriptValue.h
	M	WebCore/bindings/js/WorkerScriptController.cpp
	M	WebCore/bindings/js/WorkerScriptController.h
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/Document.h
	M	WebCore/dom/ScriptExecutionContext.h
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorResource.cpp
	M	WebCore/inspector/InspectorResource.h
	M	WebCore/workers/WorkerContext.cpp
	M	WebCore/workers/WorkerContext.h
	M	WebCore/workers/WorkerContext.idl
	A	WebCore/workers/WorkerImportScriptsClient.cpp
	A	WebCore/workers/WorkerImportScriptsClient.h
Committed r41549