Bug 22721 - Implement WorkerUtils.importScripts()
Summary: Implement WorkerUtils.importScripts()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Oliver Hunt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-07 03:21 PST by Alexey Proskuryakov
Modified: 2009-03-10 02:45 PDT (History)
1 user (show)

See Also:


Attachments
Initial pass at importScripts (29.14 KB, patch)
2009-03-06 06:55 PST, Oliver Hunt
ap: review-
Details | Formatted Diff | Diff
Updated patch (30.86 KB, patch)
2009-03-06 19:25 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Updated patch #2 (30.90 KB, patch)
2009-03-06 21:10 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Updated to match firefox behaviour, as well as a little tidy up (32.78 KB, patch)
2009-03-07 16:22 PST, Oliver Hunt
ap: review-
Details | Formatted Diff | Diff
Implement importScripts, matching spec behaviour (which appears to be the consensus) (31.77 KB, patch)
2009-03-09 22:10 PDT, Oliver Hunt
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-12-07 03:21:34 PST
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.
Comment 1 Alexey Proskuryakov 2008-12-07 03:58:37 PST
<rdar://problem/6425807>
Comment 2 Oliver Hunt 2009-03-06 06:55:39 PST
Created attachment 28358 [details]
Initial pass at importScripts

Patcheration for fun and profit
Comment 3 Alexey Proskuryakov 2009-03-06 09:21:41 PST
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!
Comment 4 Dmitry Titov 2009-03-06 13:35:52 PST
> +        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.
Comment 5 Oliver Hunt 2009-03-06 19:25:03 PST
Created attachment 28382 [details]
Updated patch
Comment 6 Oliver Hunt 2009-03-06 19:25:47 PST
Second patch removes the same origin check on scripts as the spec has had those restrictions removed
Comment 7 Oliver Hunt 2009-03-06 21:10:05 PST
Created attachment 28383 [details]
Updated patch #2

Updated to ToT so i could use ScriptExecutionContext::encoding()
Comment 8 Oliver Hunt 2009-03-06 23:49:46 PST
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
Comment 9 Oliver Hunt 2009-03-07 16:22:58 PST
Created attachment 28393 [details]
Updated to match firefox behaviour, as well as a little tidy up

Here we go
Comment 10 Alexey Proskuryakov 2009-03-09 02:03:57 PDT
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.
Comment 11 Alexey Proskuryakov 2009-03-09 02:13:42 PDT
Oh, and it looks like WorkerImportScriptsClient needs ENABLE(WORKERS) guards.
Comment 12 Alexey Proskuryakov 2009-03-09 04:09:56 PDT
+    Vector<KURL> completedUrls;

And it's URLs, not Urls.
Comment 13 Oliver Hunt 2009-03-09 22:10:50 PDT
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
Comment 14 Oliver Hunt 2009-03-09 22:54:54 PDT
In response to comments from dave_levin, i have removed the m_response field from WorkerImportScriptsClient as it was unnecessary
Comment 15 Alexey Proskuryakov 2009-03-10 00:42:20 PDT
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.
Comment 16 Oliver Hunt 2009-03-10 02:45:23 PDT
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