This bug tracks work necessary to make XMLHttpRequest available to code running in Web Workers.
There is no goal to make responseXML work yet though - per draft spec, it just returns null.
<rdar://problem/6425806>
Created attachment 26050 [details] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp This is my 1st time using git for one of these patches, so my apologies if I made a mistake with the format of the diff.
Created attachment 26138 [details] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp Modified the previous patch by not sending some messages when the inspector isn't enabled and a few more document() clean-ups in XHR.
Created attachment 26144 [details] Preliminary work for making async load available for workers. This attachment isn't intended as a patch yet. I've attached it simply to get any feedback on the shape of what I'm doing.
Comment on attachment 26138 [details] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp What is the big reason to post ScriptStrings across thread boundaries? XHR has ScriptString as data member to avoid copying - but passing data across threads involves copying anyway.
> What is the big reason to post ScriptStrings across thread boundaries? I don't understand the tradeoffs regarding why one should do something else. For example, one alternative that I can think of would be to create a String but this would involve two copies. One copy from the UString to the String, then another from the String to a UString when calling InspectorController::resourceRetrievedByXMLHttpRequest. Another alternative would be just to allocate space for the string and do a memcpy. Then before calling InspectorController::resourceRetrievedByXMLHttpRequest construct a UString take takes ownership of the space. This involves one copy (but I think it still involves another export from JavaScriptCore to export to UString constructor that takes ownership as well as a change to ScriptString to expose it). So I see two copies or a different export from JavaScriptCore as alternatives.
I think that Vector<UChar> may be more appropriate for passing XHR data between threads. This isn't about scripting, so UString/ScriptString seem out of place.
Created attachment 26246 [details] Addressed ap's comments.
-#include "PlatformString.h" I don't see why this include can be removed from ScriptString.h - operator+=(const String& s) uses the definition of String class. + bool inspectorEnabled = document()->page() ? document()->page()->inspectorController()->enabled() : false; + RefPtr<WorkerThread> thread = WorkerThread::create(m_scriptURL, userAgent, m_cachedScript->script(), inspectorEnabled, m_messagingProxy); So, Inspector will only be notified of XHR loads only if it was open when the thread was created, not if it is open when load finishes? + , m_startupData(new WorkerThreadStartupData(scriptURL, userAgent, sourceCode, inspectorEnabled)) It would be slightly nicer if WorkerThreadStartupData exposed a static create() method instead of a constructor, so that you wouldn't have to call "new" explicitly. All threading-related changes look good to me, but I don't know what the expected behavior for console logging and Inspector is (more specifically, when it is OK to omit notifications, and which messages should be inspectorOnly). I doubt that "inspector was open when the thread started" is a good check.
Created attachment 26838 [details] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp
Comment on attachment 26838 [details] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp > + Provde a generic class to handle the pattern of task callbacks across worker/parent threads. It Typo: Provde. > -#include "PlatformString.h" I still don't see why it's ok to remove this include. > +void Document::addMessageToConsole(MessageSource source, MessageLevel level, const String& message, unsigned lineNumber, const String& sourceURL, bool inspectorOnly) We're becoming more and more passionate about preferring named enums to boolean constants. > +#include "wtf/PassRefPtr.h" This should be a "<>" include. > + bool isCallbackAllowed() There is no "callback" in this context, so the name is not ideal. Maybe canPerformTask()? > + PassRefPtr<GenericWorkerTask6<P1, MP1, P2, MP2, P3, MP3, P4, MP4, P5, MP5, P6, MP6> > createCallbackTask( > + WorkerMessagingProxy* messagingProxy, > + void (*method)(ScriptExecutionContext*, MP1, MP2, MP3, MP4, MP5, MP6), I think there is a typedef for this type (Method) that you could use here. Same for return type. In general, this class is almost a generic wrapper for method invocation, having it all just for workers may be sub-optimal. I'm not requesting that you do anything about this now. > +static void AddMessageToConsoleCallback(ScriptExecutionContext* context, MessageSource source, MessageLevel level, const String& message, unsigned lineNumber, const String& sourceURL, bool inspectorOnly) Please fix this to use a lower case "a" in "add". > +void WorkerContext::addMessageToConsole(MessageSource source, MessageLevel level, const String& message, unsigned lineNumber, const String& sourceURL, bool inspectorOnly) > +{ > + PassRefPtr<Task> task; This should be RefPtr. PassRefPtr is just for passing function arguments and return values (and something that we will replace with C++0x move semantics one day in far, far future). > + // TODO(levin): The implementation is pending the fixes in https://bugs.webkit.org/show_bug.cgi?id=23175 > + ASSERT(0); We use FIXME, not TODO, and don't add commenter's name (svn blame to the rescue). You could also use notImplemented() macro, or ASSERT_NOT_REACHED(). > + void postTaskToParentContext(PassRefPtr<Task> task); // Executes the task on the parent's context's thread asynchronously. I think that you may have too many "'s" here. > + context->addMessageToConsole(JSMessageSource, ErrorMessageLevel, message, 1, String(), false); Here is the reason why we prefer named enumerations - it's not obvious at all what "false" means here. This is almost ready to land, but there are more minor nitpicks than it is practical to correct when landing, so I'll ask for one more quick round.
Created attachment 26842 [details] Two changes from the previous patch: Added back the PlatformString.h include and changed a method name.
Created attachment 26846 [details] Addressed comments form last patch. > > + PassRefPtr<GenericWorkerTask6<P1, MP1, P2, MP2, P3, MP3, P4, MP4, P5, MP5, P6, MP6> > createCallbackTask( > > + WorkerMessagingProxy* messagingProxy, > > + void (*method)(ScriptExecutionContext*, MP1, MP2, MP3, MP4, MP5, MP6), > I think there is a typedef for this type (Method) that you could use here. Same > for return type. Only within the template class. The reason why this function isn't part of the class is because C++ doesn't seem to allow the types to be inferred (without being stated explicitly). afaik, this is possible within a class. (All template parameter would be have to be stated explicitily). > In general, this class is almost a generic wrapper for method invocation, > having it all just for workers may be sub-optimal. I'm not requesting that you > do anything about this now. I see what you mean. I'll think about this but I may not get to it right away.
Created attachment 26847 [details] Addressed comments.
Comment on attachment 26847 [details] Addressed comments. + switch (destination) { + case InspectorControllerDestination: We indent cases deeper. Also, it will be slightly beneficial to replace break statements with return ones, and to add ASSERT_NOT_REACHED (gcc checks for forgotten cases, but only at compile time, not runtime). + RefPtr<Task> task; + { + String messageCopy = message.copy(); + String sourceURLCopy = sourceURL.copy(); + task = createCallbackTask(m_thread->messagingProxy(), &addMessageTask, destination, source, level, messageCopy, lineNumber, sourceURLCopy); + } + postTaskToParentContext(task.release()); I think this needs a comment explaining that we're avoiding a race condition with this construct. As discussed on IRC, this can probably be simplified if you change createCallbackTask to take const references - but you still need an explanatory comment. r=me
> We indent cases deeper. Dave pointed out that I was wrong. Oops!
Created attachment 26848 [details] Addressed comments.
Created attachment 26850 [details] Remove a few more document() from xhr code (with comments addressed).
Comment on attachment 26850 [details] Remove a few more document() from xhr code (with comments addressed). r=me + // gets called after postTaskToParentContext which causes a race condition.) TODO: Even better would be to have + // adapters for the parameters to do copies of them as appropriate, and this would also fix this issue. We don't use TODO (I'd omit this part of the comment, as it doesn't describe any bug, just a random idea we had when discussing the code).
Created attachment 26851 [details] same as above but removed TODO from comment.
Comment on attachment 26851 [details] same as above but removed TODO from comment. Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/js/ScriptString.h M WebCore/dom/Document.cpp M WebCore/dom/Document.h M WebCore/dom/ScriptExecutionContext.h M WebCore/dom/WorkerContext.cpp M WebCore/dom/WorkerContext.h M WebCore/dom/WorkerMessagingProxy.cpp M WebCore/dom/WorkerMessagingProxy.h M WebCore/dom/WorkerThread.h M WebCore/xml/XMLHttpRequest.cpp M WebCore/xml/XMLHttpRequest.h Committed r40037
Comment on attachment 26850 [details] Remove a few more document() from xhr code (with comments addressed). clearing review flag now this is landed
Comment on attachment 26851 [details] same as above but removed TODO from comment. obsolete as this is landed
Created attachment 26855 [details] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class. Some things that you may want to note: 1. I didn't make the files that I added to the xcode project "private". Some files are and some aren't and I didn't get the distinction to understand what to do. 2. didFail() got split into two methods, but the original didFail() picked up a call to internalAbort(); As far as I could tell this is all fine, but it was needed to mimic the old behavior of willSendRequest.
Comment on attachment 26855 [details] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class. + PassRefPtr<DocumentXMLHttpRequestLoader> loader = adoptRef(new DocumentXMLHttpRequestLoader(document, client, request, callbacksSetting, contentSniff)); Local variables should use RefPtr, not PassRefPtr. + ASSERT(document && client); This assertion combines two checks, so if it fails, one won't know from the printed error message which part failed. It's better to split it in two. + if (!m_loader) + return; + m_loader->clearClient(); I think that this would look better with negated check: if (m_loader) m_loader->clearClient(). + enum LoadCallbacks { + NoLoadCallbacks, + DoLoadCallbacks + }; I think that names closer to original ones could be better (maybe SendLoadCallbacks/DoNotSendLoadCallbacks?) + class XMLHttpRequestLoader : Noncopyable { + public: + static PassRefPtr<XMLHttpRequestLoader> create(ScriptExecutionContext*, XMLHttpRequestLoaderClient*, const ResourceRequest&, LoadCallbacks, ContentSniff); How can this (using PassRefPtr with a non-RefCountable class) work? As discussed on IRC, XMLHttpRequestLoader needs some refactoring, or at least renaming, to be more suitable for importScripts() use in the future.
> I didn't make the files that I added to the xcode project "private". Some > files are and some aren't and I didn't get the distinction to understand what > to do. Private headers are copied to WebCore.framework's PrivateHeaders directory, so they are available when building WebKit. If "project" didn't result in WebKit build failures, then it is the correct option to use. > didFail() got split into two methods, but the original didFail() picked up a > call to internalAbort(); I didn't quite understand this.
Created attachment 26888 [details] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class. > As discussed on IRC, XMLHttpRequestLoader needs some refactoring, or at least > renaming, to be more suitable for importScripts() use in the future. After discussion with Maciej on irc, we (really he suggested) decided upon ThreadableLoader because the key distinguishing thing about this loader is that it can be used on threads (other than the main one). > How can this (using PassRefPtr with a non-RefCountable class) work? [for ThreadableLoader] It exposes ref()/deref() methods which is all that RefPtr/PassRefPtr templates rely on. >> didFail() got split into two methods, but the original didFail() picked up a >> call to internalAbort(); > I didn't quite understand this. XHR::didFail used to look like this: void XMLHttpRequest::didFail(SubresourceLoader*, const ResourceError& error) { // If we are already in an error state, for instance we called abort(), bail out early. if (m_error) return; if (error.isCancellation()) { abortError(); return; } networkError(); return; } But the cancel case has been moved into its own method, so the code should look like this: void XMLHttpRequest::didFail(const ResourceError& error) { // If we are already in an error state, for instance we called abort(), bail out early. if (m_error) return; networkError(); } However, the willSendRequest callback has been moved into the loader to avoid bouncing between threads for this simple check. (It probably will be made an extensibility point due to importScript using the same loader.) If the willSendRequest check in the loader fails, then XMLHttpRequest::didFail is called. In order, to retain the old behavior of XMLHttpRequest::willSendRequest a call to internalAbort() is now in XHR::didFail().
Comment on attachment 26888 [details] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class. + * Abstracted away the sync and async requests behind the ThreadableLoader class, which + will get an implementation for Workers. It follows the same model as SubresourceLoader, since + it is a thin wrapper around it. I don't think this is quite accurate - DocumentThreadableLoader is a wrapper around SubresourceLoader, but ThreadableLoader is not. r=me
ap is correct about the comment in the ChangeLog. Alternative text for the ChangeLog: * Abstracted away the sync and async requests behind the ThreadableLoader class, which will get an implementation for Workers. ThreadableLoader follows the same model as SubresourceLoader because DocumentThreadableLoader is a thin wrapper around SubresourceLoader. Also, WorkerThreadableLoader (coming soon) will use DocumentThreadableLoader to get things done. I could put up a patch with this change if that works better for the lander.
Comment on attachment 26888 [details] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class. Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.pro M WebCore/WebCore.scons M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/WebCoreSources.bkl A WebCore/loader/DocumentThreadableLoader.cpp A WebCore/loader/DocumentThreadableLoader.h M WebCore/loader/SubresourceLoaderClient.h A WebCore/loader/ThreadableLoader.cpp A WebCore/loader/ThreadableLoader.h A WebCore/loader/ThreadableLoaderClient.h M WebCore/xml/XMLHttpRequest.cpp M WebCore/xml/XMLHttpRequest.h Committed r40103
Created attachment 26925 [details] Part 3: Add ability for HTTPHeaderMap to be transfered across threads.
Comment on attachment 26925 [details] Part 3: Add ability for HTTPHeaderMap to be transfered across threads. > +using std::auto_ptr; > +using std::make_pair; > +using std::pair; We usually just use "using std", unless there are conflicts. r=me
Comment on attachment 26925 [details] Part 3: Add ability for HTTPHeaderMap to be transfered across threads. Committed revision 40123, clearing review flag.
Created attachment 26927 [details] Part 4: Add ability for ResourceRequest/ResourceResponse to be transfered across threads.
Comment on attachment 26927 [details] Part 4: Add ability for ResourceRequest/ResourceResponse to be transfered across threads. > + * Copyright (C) 2009, Google Inc. All rights reserved. An extra comma here and in other modified files. What is the big difference between ResourceResponseBase and ResourceResponseData (ditto for request counterparts)? Why are separate classes necessary?
> An extra comma here and in other modified files. Working on it. > What is the big difference between ResourceResponseBase and > ResourceResponseData (ditto for request counterparts)? Why are separate classes > necessary? HTTPHeaderMap became OwnPtr<HTTPHeaderMapData>. HTTPHeaderMap can't be copied across thread due to its use of AtomicString. Perhaps I could make a common base class that both derive from with all of the data that can be copied using the same data type across threads. (I have a hard time thinking of a name for it. Perhaps ThreadableResourceResponseBase?)
Created attachment 26965 [details] Part 4: Add ability for ResourceRequest/ResourceResponse to be transfered across threads. Addressed all comments.
Comment on attachment 26965 [details] Part 4: Add ability for ResourceRequest/ResourceResponse to be transfered across threads. Looks good, r=me
Comment on attachment 26965 [details] Part 4: Add ability for ResourceRequest/ResourceResponse to be transfered across threads. Committed revision 40162.
Created attachment 26981 [details] Part 5: Add a Worker implementation for the async portion of ThreadableLoader.
Created attachment 27006 [details] Part 6: Small adjustments to the ThreadableLoader::loadResourceSynchronously to make it easier to implement for Workers
Comment on attachment 26981 [details] Part 5: Add a Worker implementation for the async portion of ThreadableLoader. + for that ar to be used on another thread. Typo: "ar". #include "ScriptExecutionContext.h" +#include "PlatformString.h" Please order includes alphabetically. + return !m_messagingProxy || !m_messagingProxy->askedToTerminate(); As discussed on IRC, I think that this can be make proxy/task lifetime unclear to someone reading the code (at a first glance, I certainly thought that you were changing it). For tasks that do not need MessageProxy to operate, it's better not to have it in the structure at all. + template<typename T> struct CrossThreadAdapter { static T& adapt(T& parameter) { return parameter; } }; + template<> struct CrossThreadAdapter<String> { static String adapt(String& parameter) { return parameter.copy(); } }; Can this be made to fail on unknown types? I think you'd need specializations for all integral types that can be copied directly, but there aren't too many in C++. +void WorkerThreadableLoader::createLoader(ScriptExecutionContext* context, WorkerThreadableLoader* workerLoader, auto_ptr<CrossThreadResourceRequestData> requestData, LoadCallbacks callbacksSetting, ContentSniff contentSniff) This function runs on the main thread, correct? I think it would be nice to ASSERT(isMainThread()) to make it clear. + workerContext->postTaskToParentContext(createCallbackTask(NULL, &WorkerThreadableLoader::doFinalDeref, this)); We use 0 in C++ code, not NULL. Will this code (and other code that posts to parent context) fail for nested workers? I think that marking such spots with FIXMEs could be helpful. +void WorkerThreadableLoader::tellClientDidReceiveData(ScriptExecutionContext*, WorkerThreadableLoader* workerLoader, char* copiedData, int lengthReceived) +{ + if (!workerLoader->messagingProxy()->askedToTerminate() && workerLoader->m_client) Why is the askedToTerminate() check only necessary in didReceiveData? What guarantees that the worker thread and context still exist when loader callbacks are sent from main thread? Maybe I didn't spend enough time thinking about it, but the loader lifetime is not clear to me. Most of the above are just questions and concerns, but there are at least a few changes to code to be made, so r- for now.
Comment on attachment 27006 [details] Part 6: Small adjustments to the ThreadableLoader::loadResourceSynchronously to make it easier to implement for Workers I don't understand why it is desired or acceptable to lose most of the information in ResourceError, reducing it to an enum. + auto_ptr<ResourceResponse> response; We use auto_ptr for passing function arguments and results, and OwnPtr for variables.
Created attachment 27070 [details] Part 5a: Add the GenericWorkTask infrastructure needed for WorkerThreadableLoader. Addressed these comments from part 5: > > + return !m_messagingProxy || !m_messagingProxy->askedToTerminate(); > For tasks that do not need MessageProxy to operate, it's better not to have it in the structure at all. > > + template<typename T> struct CrossThreadAdapter { static T& adapt(T& parameter) { return parameter; } }; > > + template<> struct CrossThreadAdapter<String> { static String adapt(String& parameter) { return parameter.copy(); } }; > Can this be made to fail on unknown types? I think you'd need specializations > for all integral types that can be copied directly, but there aren't too many > in C++. I wish I had is_enum. I couldn't figure out any easy way to do it -- (fwiw, boost has one so it is possible). Without is_enum, I had to include files that had enum's and create specializations for each enum that are used. In order to make the copying just work, I had to be able to modify the type being returned from adapt (due to ResourceRequest becoming auto_ptr<CrossThreadresourceRequestData>). Along these same lines, different type needed to be treated differently in the static create constructor. Ideally they could all be const T& but that doesn't work for auto_ptr which needs to avoid const, but T& doesn't work for returned types (from adapt), so auto_ptr<> needs to be passed in as a auto_ptr<>. Similar things happened for other types with the goal being to avoid doing an expensive/complex copy during this process.
Created attachment 27071 [details] Part 6: Small adjustments to the ThreadableLoader::loadResourceSynchronously to make it easier to implement for Workers For Part6: > I don't understand why it is desired or acceptable to lose most of the > information in ResourceError, reducing it to an enum. ResourceError would need a ::copy method due to contained strings. In addition to the overhead of this (both in code writing/maintainence/perf/etc.), XHR doesn't use any of the info (except for what is in the enum) so nothing would be testing this code at all. If anything needs more than this enum, it wouldn't be very hard to remove this enum and do the work to copy ResourceError. > + auto_ptr<ResourceResponse> response; Changed to OwnPtr. I'm not sure which one is desired here because this does get passed in as a function param but I think OwnPtr makes the most since since it is passed in as a OwnPtr<>&.
Created attachment 27101 [details] Part 5b: Add a Worker implementation for the async portion of ThreadableLoader. This is the 2nd half of what was part 5. All comments have been been directly addressed. I believe there was a lifetime issue in the code before and this has been fixed along with a comment in the code to explain how the lifetime works. It may have been fairly confusing before what methods were being used on each thread so I split the WorkerThreadableLoader into two classes with each class being used mostly on one thread to make it more clear.
Comment on attachment 27070 [details] Part 5a: Add the GenericWorkTask infrastructure needed for WorkerThreadableLoader. I don't think that relying on IsInteger is the right approach - e.g. it unnecessarily omits floating point values. I'll e-mail you some ideas to discuss. + WebCore/dom/CrossThreadAdapter.h \ The name CrossThreadAdapter is not sufficiently descriptive, it says nothing about the purpose or behavior of this class (not to mention that it does not match Adapter pattern). +++ b/WebCore/dom/CrossThreadAdapter.cpp @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2005, 2006, 2007, 2008 Apple Inc. All rights reserved. I don't see any old Apple code here. Also, did you use LGPL intentionally? I think that for other submissions, you used a BSD license. + typedef typename ComplexCopyAvoider<P1>::Type InParam1; AFAIK, "Avoider" is not a word. But I'm also not sure how this is different from CrossThreadAdapter::ReturnType - can these be merged? + template<typename T> struct ComplexCopyAvoider<T*> : public PtrType<T*> { + }; + template<typename T> struct ComplexCopyAvoider<T* const> : public PtrType<T*> { As you are removing const, you could also remove volative, as in <http://www.boost.org/doc/libs/1_37_0/libs/type_traits/doc/html/boost_typetraits/reference/remove_cv.html>. I think that behavior-wise, this is great, but the C++ magic needs another iteration or two to get it in a more maintainable shape.
Comment on attachment 27071 [details] Part 6: Small adjustments to the ThreadableLoader::loadResourceSynchronously to make it easier to implement for Workers You may be right, but I'm not convinced yet. We don't have special versions of ResourceRequest and ResourceResponse for the threadable loader, so it feels strange to have a special version of ResourceError. In fact, ResourceError is a relatively new class, as we used to have multiple ad hoc error reporting mechanisms similar to the one proposed here - and subtle mismatches between the meanings of errors were an issue. Our XMLHttpRequest implementation is not sufficiently mature when it comes to error handling (see bug 17426, bug 13075 and some others), so it is quite likely that more information from ResourceError will be needed. I think that the names are not sufficiently descriptive - e.g. when you write "if (!document->frame()) errorType = FailedLoaderError;" it seems that the problem is that there is no loader, so I'd expect to see "NoLoaderError" here. But that's not what NoLoaderError means! r-, due to at least the names.
Comment on attachment 27101 [details] Part 5b: Add a Worker implementation for the async portion of ThreadableLoader. Discussed this on IRC, there are some changes to be made to the lifetime model here. r- to get it out of review queue.
Comment on attachment 27070 [details] Part 5a: Add the GenericWorkTask infrastructure needed for WorkerThreadableLoader. Replaced by https://bugs.webkit.org/show_bug.cgi?id=23618
Comment on attachment 27101 [details] Part 5b: Add a Worker implementation for the async portion of ThreadableLoader. Replaced by Bug 23636: Make the async api of ThreadableLoader functional for the worker context.
Comment on attachment 27071 [details] Part 6: Small adjustments to the ThreadableLoader::loadResourceSynchronously to make it easier to implement for Workers Replaced by https://bugs.webkit.org/show_bug.cgi?id=23688
There is one blocker left, which is basically about Web Inspector support. It is important, but not really a reason to say that we don't support XHR in Workers. Is there anything else that needs to be done to consider this resolved?
This is in the build and working. Layout tests are being added and bugs being fixed. The one bug that this depends on is about exposing the xhr result to the inspector.