Bug 28714 – Need to write tests for worker lifecycle mechanisms

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

We don't yet have any tests that cover worker lifecycle management.

In particular, we don't test whether threads exit under the following conditions:

1) dedicated workers orphaned (no references in parent document)
2) fire-and-forget workers (orphaned workers that fire off an async operation and so should exit once the operation completes)
3) shared workers exiting when their parent document closes
4) shared workers *not* exiting when their parent document closes if they still have an active parent somewhere

Now that we expose the number of running workers in layoutTestController.workerThreadCount, it should be possible to test all of these cases.

[Andrew Wilson]

Created attachment 38557 [details]
patch containing new layout tests

This patch has no code changes, only new/refactored tests.

[David Levin]

Comment on attachment 38557 [details]
patch containing new layout tests



> diff --git a/LayoutTests/fast/workers/resources/create-shared-worker-frame.html b/LayoutTests/fast/workers/resources/create-shared-worker-frame.html
> \ No newline at end of file

Please fix.

> diff --git a/LayoutTests/fast/workers/resources/worker-util.js b/LayoutTests/fast/workers/resources/worker-util.js

> +function gc()
> +{
> +    for (var i = 0; i < 10000; i++) { // force garbage collection (FF requires about 9K allocations before a collect)
> +        var s = new String("abc");
> +    }

I think there is a layoutTestController.gc() function that you may want to use if it is there.

 
> +function waitUntilThreadCountMatches(callback, count)
...
> +        setTimeout(function() { waitUntilThreadCountMatches(callback, count); }, 100);

A shorter poll interval would be good to ensure that the test runs quickly.


> +function ensureThreadCountMatches(callback, count)
> +{
> +    // Just wait until the thread count matches, then wait another 100ms to see if it changes, then fire the callback.

100ms is a bit long for a unit test.  Is there any way to make this smaller?

> +    waitUntilThreadCountMatches(function() {
> +            setTimeout(function() { waitUntilThreadCountMatches(callback, count); },  100);

You have an extra space before the 100.


> +    if (window.layoutTestController && window.layoutTestController.notifyDone)

Seems odd to check "window.layoutTestController.notifyDone"?

[Andrew Wilson]

(In reply to comment #2)

> 
> I think there is a layoutTestController.gc() function that you may want to use
> if it is there.

Hmmm, my intent was to use that, but I guess it wasn't in the version of the routine I copy/pasted. Yet another reason to have a central copy in a utility file like this, I guess. I'll fix this up.

> 
> 
> > +function waitUntilThreadCountMatches(callback, count)
> ...
> > +        setTimeout(function() { waitUntilThreadCountMatches(callback, count); }, 100);
> 
> A shorter poll interval would be good to ensure that the test runs quickly.

OK, I'll change the poll timer to 10ms.

> 
> 
> > +function ensureThreadCountMatches(callback, count)
> > +{
> > +    // Just wait until the thread count matches, then wait another 100ms to see if it changes, then fire the callback.
> 
> 100ms is a bit long for a unit test.  Is there any way to make this smaller?

I'm not sure how. The point of the code is to make sure that we've hit a "steady-state" in the number of threads, so I have to give it time for other threads to possibly exit. If you have any ideas on how to improve this, let me know - I'm concerned about dropping this below 100ms as smaller values might not give other threads time to exit, allowing the test to pass inappropriately.

> 
> > +    if (window.layoutTestController && window.layoutTestController.notifyDone)
> 
> Seems odd to check "window.layoutTestController.notifyDone"?

Yeah, I was trying to make these tests run in a browser by trying to mock up a version of window.layoutTestController. I eventually abandoned this in favor of a different approach, but I must've forgotten to remove this check. I'll fix this.

Thanks for the feedback!

[Andrew Wilson]

Committed as r47837.