Bug 75529 – [Qt] SharedWorkers do not terminate after tests

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Nandor Huszka]

Created attachment 121079 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp

After a test is finished, DRT should check if there is a hanging worker and throw some error message.
I coded a checking in DumpRenderTree::dump in DumpRenderTreeQt.cpp. It tests the number of living workers in a loop, and if it is positive, calls the GC until there are stucked workers, but maximum five times. After all, DRT prints an error message with the number of workers, if they still live.
88 tests failed with this error. Essentially the cause of failing that they do not terminate the used workers. There is an attachment with the fixing (and the modification in DRT), it works well in 67 cases. An example:
...
var worker = new Worker(url);
...
function finishTest() {
    worker.terminate();
    // this function can be found in worker-util.js
    waitUntilWorkerThreadsExit(function() {
        if (window.layoutTestController)
            layoutTestController.notifyDone();
    });
}

SharedWorker does not have terminate method, it can only exits after there is no reference for it. I tried this when a test use a SharedWorker:
...
var worker = new SharedWorker(url);
...
function finishTest() {
    worker=null;
    // force a garbage collection
    gc();
    waitUntilWorkerThreadsExit(function() {
        if (window.layoutTestController)
            layoutTestController.notifyDone();
    });
}
In this case the test fails with timeout, because its worker does not terminate. Worker's reference was not passed, was not used any other place. Do yo have any idea why do not exit SharedWorkers?

[Nandor Huszka]

Created attachment 121081 [details]
The path of tests which use SharedWorkers

[Nandor Huszka]

Created attachment 121083 [details]
An example for a test which use a SharedWorker. It contains the mentioned fixing, which does not work.

Its path:
fast/workers/shared-worker-simple.html

[Nandor Huszka]

We had a discussion with Ossy, as a result the tests which are using Workers will be refixed, because there are dependency problems around JavaScript test importing. However I still do not know why SharedWorkers do not terminate.

[Nandor Huszka]

Created attachment 121422 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp 

I reorganized the fixing, some useful things were created in worker-util.js, and used them. For example a global array (workerArray), it's recommended to use this for storing workers, because in some cases I found problems around referencing the created workers.

[WebKit Review Bot]

Attachment 121422 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/files/workers/inline-work..." exit_code: 1

Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:955:  Tab found; better to use spaces  [whitespace/tab] [1]
Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:956:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nandor Huszka]

Created attachment 121632 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp

Indentation is corrected.

[WebKit Review Bot]

Comment on attachment 121632 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp

Attachment 121632 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11185475

New failing tests:
http/tests/eventsource/workers/eventsource-simple.html
http/tests/inspector/extensions-headers.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-cd.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/extensions-ignore-cache.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/compiler-source-mapping.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector/change-iframe-src.html

[Nandor Huszka]

Created attachment 121661 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp 

Fix

[David Levin]

Comment on attachment 121661 [details]
The fixing of the tests which use "normal" Workers and the checking in DumpRenderTreeQt.cpp 

There are several problems with the patch.
1. It is missing a change log which would hopefully explain why some changes are being done (see other issues below).
2. There are formatting/indenting issues.
3. There is a notifyWhen function which doesn't have a callback so it doesn't notify.
4. There are a lot of tests which only have this added "<script src="resources/worker-util.js"></script>" and it is unclear why.
5. It is unclear to me why the tests needed the changes that they got (like the worker array).
6. This is a rather large change and really does several different things. Hopefully it would be broken up into different patches that do things separately and this would be more easily and quickly reviewed. (For example the drt c++ changes could be one change.)

This is just what I noticed quickly. There may be other issues once these get addressed.

[Nandor Huszka]

(In reply to comment #9)
> (From update of attachment 121661 [details])
> There are several problems with the patch.
> 1. It is missing a change log which would hopefully explain why some changes are being done (see other issues below).
> 2. There are formatting/indenting issues.

The r? really was not justified, EWS analysis what I just needed, because it is a WIP patch. It tries to terminate all the stucked Workers, but it will not resolve the original SharedWorker problem. Whatever, thank you for the comments, the explanations will be added to the ChangeLog too.

> 3. There is a notifyWhen function which doesn't have a callback so it doesn't notify.

The layoutTestController.notifyDone calling can be found in many tests, and before this all the Workers should be terminated. The notifyDoneWhenWorkersFreed function do this exactly, it exits all the used Workers (which are stored in the workerArray), and then calls the layoutTestController.notifyDone method.

> 4. There are a lot of tests which only have this added "<script src="resources/worker-util.js"></script>" and it is unclear why.
> 5. It is unclear to me why the tests needed the changes that they got (like the worker array).

In my opinion, the global workerArray is needed. Workers frequently declared as a local variable in a function, but they should be terminated in an another function. Also frequently occurs that there is possiblity to terminate workers just in an another JavaScript file. For example a worker is created by dedicated-workers-list.html, but terminating it is needed in inspector-test.js, which is imported by dedicated-workers-list.html. If worker is stored as a standard variable, we cannot know its name in inspector-test.js. A global worker array is recommended for storing workers, with this terminating workers in another files is safer.

> 6. This is a rather large change and really does several different things. Hopefully it would be broken up into different patches that do things separately and this would be more easily and quickly reviewed. (For example the drt c++ changes could be one change.)

In itself the modification in DRT causes some test's failing, because it puts an error message to the output, if a test leaves living worker after it. The purpose of the cpp code: tests authors pay attention to terminate the used workers.

It is not a complete patch this way, because SharedWorkers do not terminate, it is in question yet.

[David Levin]

(In reply to comment #10) 
> The r? really was not justified, EWS analysis what I just needed, because it is a WIP patch.

Please indicate when something is a work in progress and the r? is just there to get a ews run.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.