Bug 62292 – [Chromium] Worker object may be garbage collected even if it has message handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yury Semikhatsky]

Currently if you create a dedicated worker by calling the following function:

function createWorker() {
    var worker = new Worker("worker.js");
    worker.onmessage = function(event) {
       console.log("Message from worker: " + event.data);
    };
    worker.postMessage("ping");
}

the worker object may well be GC'ed before any message from the worker
context is received. This behavior looks confusing. The spec doesn't say
anything specific about this while in many examples there no references
to the worker object to which onmessage/onerror handlers are attached which
means that those samples may be affected by this issue.

Should we protect the worker from being collected while it has message
handlers? Probably this problem has already been discussed.

[Dmitry Titov]

Is there a repro case for this?

[Alexey Proskuryakov]

In your example, the worker object will be GC protected because there is an outstanding message. Once it's handled and acknowledged, and we also know that it hasn't triggered a timer or XHR inside the worker thread, it should be fine to collect the worker, because we know that it can't possibly be messaged again from either side.

Having handlers always GC protect the object would make it extremely likely to leak.

So, this is correct behavior. Do you have an example where the implementation doesn't match the design that I described above?

[Andrey Kosyakov]

Doesn't fast/workers/worker-gc.html aim at testing just this? I wonder what's so different in Yury's example.

[Dmitry Titov]

(In reply to comment #3)
> Doesn't fast/workers/worker-gc.html aim at testing just this? I wonder what's so different in Yury's example.

During test in worker-gc.html, the Worker always has 'pending activity' in the form of a message being sent and replied. 

In the Yury.s example, I would expect the "ping" message to be delivered to the worker context, and if the worker context replies with another message from its onmessage handler, it would be delivered back and appended to console log. After that, if there is no pending activity in the worker context (no timers, xhrs, database requests, file operations) - the worker becomes 'unprotected' (as spec says) in a sense that it can not fire any new events and thus can be GC'ed safely, together with its event handlers.

However, if in this example the worker context replies back but the even handler is not firing because it was GC'ed, it's a bug.

[Yury Semikhatsky]

Created attachment 96551 [details]
Repro case 1

[Yury Semikhatsky]

Created attachment 96552 [details]
Repro case 1

[Yury Semikhatsky]

(In reply to comment #1)
> Is there a repro case for this?

Yes, see attached worker.html and worker.js. worker.js periodically sends a message to its worker object so the worker context is not collected. However the worker object is not GC protected and I see only part of the messages printed in the page. I can reliably reproduce it on Chromium Linux after the page refresh: messages from worker #0 will never be printed.

[Alexey Proskuryakov]

I cannot reproduce this in WebKit nightly r88132 (after changing the test to not use bind()). I forced garbage collection several times manually when running the test.

[Yury Semikhatsky]

(In reply to comment #2)
> In your example, the worker object will be GC protected because there is an outstanding message. Once it's handled and acknowledged, and we also know that it hasn't triggered a timer or XHR inside the worker thread, it should be fine to collect the worker, because we know that it can't possibly be messaged again from either side.
> 
Does it mean that the worker object should stay alive while there is a pending activity in the corresponding worker context?

> Having handlers always GC protect the object would make it extremely likely to leak.
> 
Yeah, I understand that. As I see in current implementation tracking of pending activities in the worker context is supposed to address the issue.

> So, this is correct behavior. Do you have an example where the implementation doesn't match the design that I described above?
I posted my example, it looks like a bug in Chromium's implementation which appears after page refresh.

[Yury Semikhatsky]

(In reply to comment #8)
> I cannot reproduce this in WebKit nightly r88132 (after changing the test to not use bind()). I forced garbage collection several times manually when running the test.

I suspect this is Chromium-specific, let me try to reproduce it on WebKit ToT.

[Yury Semikhatsky]

(In reply to comment #10)
> (In reply to comment #8)
> > I cannot reproduce this in WebKit nightly r88132 (after changing the test to not use bind()). I forced garbage collection several times manually when running the test.
> 
> I suspect this is Chromium-specific, let me try to reproduce it on WebKit ToT.

Yeah, I couldn't reproduce it on WebKit r88431 Linux Qt.

[Yury Semikhatsky]

Created attachment 96582 [details]
Reduced test case

The case doesn't fail in debug mode with a shorter interval in worker-pending.js as in that case message from the worker context seems to be delivered before acknowledge for the message from the worker.

[Yury Semikhatsky]

Created attachment 96587 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #13)
> Created an attachment (id=96587) [details]
> Patch

I'd like to convert the test case into a layout test but I don't see yet how to do this so that the test isn't too slow and reliable. The problem with test is that full-GC needs to be triggered right after WebWorkerClientImpl::confirmMessageFromWorkerObject is called and there is no way to hook WebWorkerClientImpl::confirmMessageFromWorkerObject from JavaScript.

[Alexey Proskuryakov]

Did you try forcing garbage collection to make the test faster? There is a gc() function in js-test-pre.js.

[Yury Semikhatsky]

WebWorkerClientImpl currently inverts order of dispatching of confirmMessageFromWorkerObject and postMessageToWorkerObject.
Let assume that WebWorkerClientImpl::postMessageToWorkerObject is called right after WebWorkerClientImpl::confirmMessageFromWorkerObject. WebWorkerClientImpl::confirmMessageFromWorkerObject will unconditionally schedule asynchronous task while WebWorkerClientImpl::postMessageToWorkerObject will check if current thread is the same where the client was created (which is always true for workers created in Page) and will dispatch the message synchronously i.e. before WebWorkerClientImpl::confirmMessageFromWorkerObjectTask is executed.

Looks like a bug to me. Dmitry, could clarify this?

[Yury Semikhatsky]

(In reply to comment #15)
> Did you try forcing garbage collection to make the test faster? There is a gc() function in js-test-pre.js.

Yes, tried it both by inserting:

    function triggerGC() {
        var head;
        for (var i = 0; i < 10000; i++) {
            var newHead = new Array(1000);
            newHead[0] = head;
            head = newHead;
        }
    }
    setInterval(triggerGC, 0);

and 


    function triggerGC() {
        var head;
        for (var i = 0; i < 10000; i++) {
            var newHead = new Array(1000);
            newHead[0] = head;
            head = newHead;
        }
        gc();
    }
    setInterval(triggerGC, 0);

with no success. If the interval between acknowledge and message from worker context is short enough, we cannot be sure that triggerGC() is invoked in between.

[Dmitry Titov]

(In reply to comment #16)
> WebWorkerClientImpl currently inverts order of dispatching of confirmMessageFromWorkerObject and postMessageToWorkerObject.
> Let assume that WebWorkerClientImpl::postMessageToWorkerObject is called right after WebWorkerClientImpl::confirmMessageFromWorkerObject. WebWorkerClientImpl::confirmMessageFromWorkerObject will unconditionally schedule asynchronous task while WebWorkerClientImpl::postMessageToWorkerObject will check if current thread is the same where the client was created (which is always true for workers created in Page) and will dispatch the message synchronously i.e. before WebWorkerClientImpl::confirmMessageFromWorkerObjectTask is executed.
> 
> Looks like a bug to me. Dmitry, could clarify this?

I think there is no bug there, at least at first look. It's fine to delay confirmation, it only will keep the Worker object alive for a bit longer.

However, loosing the pending activity information that you are fixing in your patch is a real bug. Very nice catch!

Note that this code will soon be eliminated ([e-mail] works on moving dedicated workers back in process so they won't use Chromium WebWorker API anymore. Shared workers will still go via Chromium API, but they have different lifetime tracking. This work will also enable running dedicated workers tests in DRT for Chromium port (currently all workers layout tests are disabled in test_expectations) - so unless you want to add a ui_test for this failure, I don't see how it can be tested.

So I'm going to r+ your patch w/o test.

[Dmitry Titov]

Comment on attachment 96587 [details]
Patch

r=me. Please add a comment to ChangeLog on why there is no layout test, and rename the bug to have '[Chromium]' in it, including in ChangeLog.

[Yury Semikhatsky]

Created attachment 96703 [details]
Patch for landing

Changelog entry was modified to include information about testing the change.

[Yury Semikhatsky]

Committed r88529: <http://trac.webkit.org/changeset/88529>

[Yury Semikhatsky]

(In reply to comment #18)
> (In reply to comment #16)
> Note that this code will soon be eliminated ([e-mail] works on moving dedicated workers back in process so they won't use Chromium WebWorker API anymore. 

Is it in a usable state yet? Any ETA? Then I'd rather focus on the inspection of the new in-process dedicated workers instead of spending time on the current implementation which involves modifications in Chromium IPC.

[Dmitry Lomov]

(In reply to comment #22)
> (In reply to comment #18)
> > (In reply to comment #16)
> > Note that this code will soon be eliminated ([e-mail] works on moving dedicated workers back in process so they won't use Chromium WebWorker API anymore. 
> 
> Is it in a usable state yet? Any ETA? Then I'd rather focus on the inspection of the new in-process dedicated workers instead of spending time on the current implementation which involves modifications in Chromium IPC.

My ETA for this is end of June. You can track progress at https://bugs.webkit.org/show_bug.cgi?id=61016