Bug 24330 - Sync xhr in workers should send an abort exception when the worker is terminated.
Summary: Sync xhr in workers should send an abort exception when the worker is termina...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-03 10:48 PST by David Levin
Modified: 2009-03-04 14:31 PST (History)
1 user (show)

See Also:


Attachments
Proposed fix. (6.74 KB, patch)
2009-03-03 11:13 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-03-03 10:48:06 PST
See summary.
Comment 1 David Levin 2009-03-03 11:13:24 PST
Created attachment 28232 [details]
Proposed fix.


2009-03-03  David Levin  <levin@chromium.org>

        Reviewed by NOBODY (OOPS!).

        Bug 24330: Sync xhr in workers should send abort error when the worker is terminated.
        <https://bugs.webkit.org/show_bug.cgi?id=24330>

        Test: http/tests/xmlhttprequest/workers/abort-error-assert.html

        * dom/ExceptionCode.cpp:
        (WebCore::xmlHttpRequestExceptionNames):
        Added missing ABORT_ERR whse absence caused an assert.

        * loader/WorkerThreadableLoader.cpp:
        (WebCore::WorkerThreadableLoader::loadResourceSynchronously):
        (WebCore::WorkerThreadableLoader::MainThreadBridge::cancel):
        Add more logic to handle the termination case for sync xhr.

LayoutTests:

2009-03-03  David Levin  <levin@chromium.org>

        Reviewed by NOBODY (OOPS!).

        Bug 24330: Sync xhr in workers should send abort error when the worker is terminated.
        <https://bugs.webkit.org/show_bug.cgi?id=24330>

        Added test to verify to terminate a worker while it is doing a synchronous xhr.
        It does not verify that the exception is a ABORT_ERR because I couldn't figure out a way
        to actually do this.

        * http/tests/xmlhttprequest/workers/abort-error-assert-expected.txt: Added.
        * http/tests/xmlhttprequest/workers/abort-error-assert.html: Added.
        * http/tests/xmlhttprequest/workers/resources/endless-response.php: Copied from LayoutTests/http/tests/xmlhttprequest/resources/endlessxml.php.
        * http/tests/xmlhttprequest/workers/resources/endless-sync-xhr.js: Added.
---
 LayoutTests/ChangeLog                              |   16 +++++++++
 .../workers/abort-error-assert-expected.txt        |    5 +++
 .../xmlhttprequest/workers/abort-error-assert.html |   34 ++++++++++++++++++++
 .../workers/resources/endless-response.php         |    3 ++
 .../workers/resources/endless-sync-xhr.js          |    7 ++++
 WebCore/ChangeLog                                  |   18 ++++++++++
 WebCore/dom/ExceptionCode.cpp                      |    3 +-
 WebCore/loader/WorkerThreadableLoader.cpp          |   18 +++++++++-
 8 files changed, 101 insertions(+), 3 deletions(-)
Comment 2 Alexey Proskuryakov 2009-03-04 01:20:04 PST
Comment on attachment 28232 [details]
Proposed fix.

+        Bug 24330: Sync xhr in workers should send abort error when the worker is terminated.

"Should raise an exception" maybe? But what really counts is that we shouldn't keep waiting for load to finish, as the various ways to stop execution are indistinguishable.

+        // This is a small race condition here in that the worker may not have started the sync xhr call at this point,
+        // but I couldn't find a way to eliminate it.

Maybe wait 100ms to make it less likely?

\ No newline at end of file

Please add one.

+    if (!loader->done() && result == MessageQueueTerminated) {
+        loader->cancel();
+    }

No braces around single line blocks.

+        // If the client hasn't finished at this point, then send a cancellation error because there will be no
+        // more callbacks from the main thread.

As discussed on IRC, this comment could be better.

r=me
Comment 3 David Levin 2009-03-04 14:31:15 PST
Committed as r41433.