Bug 27924 - Need to test throwing exceptions from Workers after calling close()
Summary: Need to test throwing exceptions from Workers after calling close()
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
Depends on:
Reported: 2009-08-01 12:58 PDT by Andrew Wilson
Modified: 2009-08-05 22:19 PDT (History)
1 user (show)

See Also:

patch containing layout test (2.93 KB, patch)
2009-08-01 13:05 PDT, Andrew Wilson
levin: review-
Details | Formatted Diff | Diff
Patch addressing levin's comments (2.95 KB, patch)
2009-08-04 11:43 PDT, Andrew Wilson
fishd: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-08-01 12:58:56 PDT
After we call close(), any exceptions thrown from Worker context should still be propagated back to page context via the Worker.onerror handler.

This seems to work properly, but we should add a layout test to confirm this.
Comment 1 Andrew Wilson 2009-08-01 13:05:41 PDT
Created attachment 33940 [details]
patch containing layout test

Patch only contains layout tests, since the code seems to work as-is.
Comment 2 David Levin 2009-08-03 23:13:29 PDT
Comment on attachment 33940 [details]
patch containing layout test

Just a few minor things to consider or address which will make it possible to land this easily

> diff --git a/LayoutTests/fast/workers/resources/worker-close.js b/LayoutTests/fast/workers/resources/worker-close.js
> @@ -22,6 +22,9 @@ onmessage = function(evt)
>          postMessage("pong");
>      } else if (evt.data == "throw") {
>          throw "should never be executed";
> +    } else if (evt.data == "closeWithError") {
> +        close();
> +        generateError();  // Undefined function - throws exception

Even time I see this I want to look for the generateError function.  Just an idea but what do you think about calling it nonExistantFunction();

> diff --git a/LayoutTests/fast/workers/worker-close.html b/LayoutTests/fast/workers/worker-close.html
> +    // Test that errors are delivered after close

Please end with "."

> +    worker = new Worker('resources/worker-close.js');
> +    worker.postMessage("closeWithError");
> +    worker.onerror = function(event) {
> +        log("PASS: Error arrived after close: " + event.message);
> +        testPendingEvents();
> +	return false;

Indentation is off.

> +    }
> +}
Comment 3 Andrew Wilson 2009-08-04 11:43:15 PDT
Created attachment 34077 [details]
Patch addressing levin's comments

That indentation error was due to a tab somehow getting into the code. Weird. Maybe I used vi to edit a file inadvertently...
Comment 4 Adam Barth 2009-08-05 22:19:47 PDT
Comment on attachment 34077 [details]
Patch addressing levin's comments

Clearing review flag on attachment: 34077

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/workers/resources/worker-close.js
	M	LayoutTests/fast/workers/worker-close-expected.txt
	M	LayoutTests/fast/workers/worker-close.html
Committed r46830
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/workers/worker-close.html
	M	LayoutTests/fast/workers/worker-close-expected.txt
	M	LayoutTests/fast/workers/resources/worker-close.js
r46830 = 515c284335367a97d1e0796b7395e6c92eeaea89 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
Comment 5 Adam Barth 2009-08-05 22:19:51 PDT
All reviewed patches have been landed.  Closing bug.