Bug 23688 - ThreadableLoader needs a sync implementation for Workers.
Summary: ThreadableLoader needs a sync implementation for Workers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 24089
Blocks: 22720
  Show dependency treegraph
 
Reported: 2009-02-02 11:39 PST by David Levin
Modified: 2009-02-25 10:08 PST (History)
2 users (show)

See Also:


Attachments
Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously (4.31 KB, patch)
2009-02-02 18:15 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 1: Put ResourceError in ThreadableLoaderClient (13.88 KB, patch)
2009-02-04 02:19 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 1: Put ResourceError in ThreadableLoaderClient (deleted)
2009-02-11 07:37 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Add a way to post a task and synchronously wait for it to finish. (7.18 KB, patch)
2009-02-12 04:04 PST, David Levin
ap: review-
Details | Formatted Diff | Diff
Part 3: Add the sync implementation. (27.09 KB, patch)
2009-02-12 04:40 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Add a way to post a task and wait for its algorithm to finish. (8.20 KB, patch)
2009-02-12 21:51 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Add the sync implementation for Workers. (11.43 KB, patch)
2009-02-25 01:38 PST, David Levin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-02-02 11:39:17 PST
Part of the work for making xhr available to workers.
Comment 1 David Levin 2009-02-02 18:15:07 PST
Created attachment 27272 [details]
Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously
Comment 2 David Levin 2009-02-03 00:00:13 PST
Comment on attachment 27272 [details]
Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously

Due to comments from ap.  I'm scrapping this patch.
Comment 3 David Levin 2009-02-04 02:19:20 PST
Created attachment 27311 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

The async api needs to expose ResourceError so that the sync api can return it (since the sync api is a thin wrapper around the async version).

blockedError is a rather arbitrary choice for these cases but it seemed like a good choice for these cases.
Comment 4 Alexey Proskuryakov 2009-02-04 07:30:14 PST
Comment on attachment 27311 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

This looks good to me, except for using a special value for using a special error value for "couldn't create a loader". Mixing loader callbacks and a "could not create a loader" callback seems misleading. I'm not sure if this can ever happen in practice, so maybe a comment would suffice.
Comment 5 David Levin 2009-02-04 09:09:14 PST
The failed to create loader can happen in three places (in SubresourceLoader::create):

    if (!frame)
        return 0;
// I don't care about this one. I believe it means that that the window is shutting down.


    FrameLoader* fl = frame->loader();
    if (!skipCanLoadCheck && fl->state() == FrameStateProvisional)
        return 0;

// I don't know if this one is possible.

    ResourceRequest newRequest = request;

    if (!skipCanLoadCheck
            && FrameLoader::restrictAccessToLocal()
            && !FrameLoader::canLoad(request.url(), String(), frame->document())) {
        FrameLoader::reportLocalLoadFailed(frame, request.url().string());
        return 0;
    }

// This one should be possible if a http: page tries to load file://

In the sync case,  that happens here:
  XMLHttpRequest::processSyncLoadResults
  {
      if (m_sameOriginRequest && !scriptExecutionContext()->securityOrigin()->canRequest(response.url())) {
          abort();
          return;
      }

I can add a special method in ThreadableLoaderClient for the failed to create loader errors. fwiw, the issue isn't the DocumentThreadableLoader but instead the WorkerThreadableLoader.  In the case of the WorkerThreadableLoader, the failure to create the subresourceloader happens async.


Comment 6 David Levin 2009-02-06 08:33:16 PST
Comment on attachment 27311 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

Need to make some improvements based on ap's suggestions.  (Moving out of review queue while I do this.)
Comment 7 David Levin 2009-02-11 07:37:13 PST
Created attachment 27562 [details]
Part 1: Put ResourceError in ThreadableLoaderClient
Comment 8 Alexey Proskuryakov 2009-02-12 00:29:26 PST
Comment on attachment 27562 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

+    // FIXME: If the a site requests a local resource, then this will return a non-zero value but the sync path
+    // will return a 0 value.  Either this should return 0 or the other code path should do a callback with
+    // a failure.
     thisPtr->m_mainThreadLoader = ThreadableLoader::create(context, thisPtr, *request, callbacksSetting, contentSniff);
-    if (!thisPtr->m_mainThreadLoader)
-        thisPtr->didFail();

Seems that we should assert that the result is non-null here.

r=me
Comment 9 Alexey Proskuryakov 2009-02-12 00:33:28 PST
Comment on attachment 27562 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

Committed revision 40889, clearing review flag.
Comment 10 David Levin 2009-02-12 04:04:46 PST
Created attachment 27592 [details]
Part 2: Add a way to post a task and synchronously wait for it to finish.
Comment 11 David Levin 2009-02-12 04:40:05 PST
Created attachment 27595 [details]
Part 3: Add the sync implementation.
Comment 12 Alexey Proskuryakov 2009-02-12 05:08:48 PST
Comment on attachment 27592 [details]
Part 2: Add a way to post a task and synchronously wait for it to finish.

+class TaskForWorkerContextBlockingCondition : public ScriptExecutionContext::Task {

Let's try a round of making a better name - this one doesn't really parse to me.

+        // The signaling cannot be done at the end of this method because the task may finish asynchronously.

Would it make sense to unconditionally signal from task destructor?

There is a race condition between postTaskToWorkerObjectAndWait() and unblockWorkerContext(): if the task finishes and signals before calling secondary thread enters its wait, it will never be unblocked (pthread_cond_signal() has no effect if no threads are waiting). A common pattern is to synchronize on the mutex that is passed to wait().

+        void unblockWorkerContext(); // unblocks postTaskToWorkerObjectAndWait

We prefer full sentence comments.

+        // when it has finished its work.  WorkerMessagingProxy::unblockWorkerContext will in turn signal m_syncFlag.

I believe that the prevailing style is not to use French spacing (i.e., we don't put two spaces between sentences).

As discussed on IRC, this design will not allow for progress events to fire while a sync request is running. Per XHR spec, at least readystatechange should fire - it's an open issue whether progress events should, too. And I'm not sure if timers are supposed fire while a request is in progress. But a correct implementation of events can wait until later.

r- due to the race condition.
Comment 13 Alexey Proskuryakov 2009-02-12 05:17:25 PST
Comment on attachment 27595 [details]
Part 3: Add the sync implementation.

> +        * loader/LoaderForWaitingWorkerContext.cpp: Added.

Hmm - somehow, I assumed that sync and async could share the same loader on the main thread.
Comment 14 David Levin 2009-02-12 21:51:10 PST
Created attachment 27640 [details]
Part 2: Add a way to post a task and wait for its algorithm to finish.

Addressed comments.
Comment 15 David Levin 2009-02-13 00:55:12 PST
Comment on attachment 27595 [details]
Part 3: Add the sync implementation.

I'm going to revisit this approach after some comments by ap.
Comment 16 David Levin 2009-02-25 01:38:19 PST
Created attachment 27962 [details]
Part 2: Add the sync implementation for Workers.
Comment 17 Alexey Proskuryakov 2009-02-25 03:38:33 PST
Comment on attachment 27962 [details]
Part 2: Add the sync implementation for Workers. 

 void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, const ResourceRequest& request, ThreadableLoaderClient& client)
 {
     ASSERT(context);
+
+#if ENABLE(WORKERS)
+    if (context->isWorkerContext())
+        return WorkerThreadableLoader::loadResourceSynchronously(static_cast<WorkerContext*>(context), request, client);

Even though this works, I'd put "return" on a separate line.

+    ContentSniff contentSniff =  request.url().isLocalFile() ? SniffContent : DoNotSniffContent;

Two spaces after "=".

r=me
Comment 18 David Levin 2009-02-25 10:08:43 PST
Committed in r41216.