Bug 23688 – ThreadableLoader needs a sync implementation for Workers.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Levin]

Part of the work for making xhr available to workers.

[David Levin]

Created attachment 27272 [details]
Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously

[David Levin]

Comment on attachment 27272 [details]
Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously

Due to comments from ap.  I'm scrapping this patch.

[David Levin]

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.

[Alexey Proskuryakov]

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.

[David Levin]

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.

[David Levin]

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.)

[David Levin]

Created attachment 27562 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Comment on attachment 27562 [details]
Part 1: Put ResourceError in ThreadableLoaderClient

Committed revision 40889, clearing review flag.

[David Levin]

Created attachment 27592 [details]
Part 2: Add a way to post a task and synchronously wait for it to finish.

[David Levin]

Created attachment 27595 [details]
Part 3: Add the sync implementation.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[David Levin]

Created attachment 27640 [details]
Part 2: Add a way to post a task and wait for its algorithm to finish.

Addressed comments.

[David Levin]

Comment on attachment 27595 [details]
Part 3: Add the sync implementation.

I'm going to revisit this approach after some comments by ap.

[David Levin]

Created attachment 27962 [details]
Part 2: Add the sync implementation for Workers.

[Alexey Proskuryakov]

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

[David Levin]

Committed in r41216.