1451381 - Worker event listeners can leak owning window

Status: NEW

(Core :: DOM: Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1450358 I am adding some new runtime leak checking.  The Worker API appears to fail this test.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test Worker API for event listener leaks. r=baku

This test needs the infrastructure patches from bug 1450358 to work.  Currently the Worker API fails all three leak cases.

Andrea, can you take a look at what is going on?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I tried writing a simple DisconnectFromOwner() that did Terminate(), but that doesn't fix the leak here.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Here a patch but I don't think we should land the test because it will be probably intermittent: the releasing of the window happens when the worker ends its shutdown steps. This could take more than a couple of ForceGC() calls.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8969341 [details] [diff] [review]
patch

Review of attachment 8969341 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't we call Terminate() from DisconnectFromOwner()?  Otherwise the worker thread will run between the window being closed and CC running.

Also, if you don't like the test using event_leak_utils.js, can you write another test like it that waits for the worker to shutdown?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #4)
> Also, if you don't like the test using event_leak_utils.js, can you write
> another test like it that waits for the worker to shutdown?

For example, you could have a test where the worker spins periodically calling BroadcastChannel.postmessage().  Then the test could listen for when the broadcast messages stop before triggering GC.

Comment 6

[Andrea Marchesini [:baku]]

Ok... this is going to be interesting: the current worker code doesn't ends the event-loop when all the 'blocking' holders are released, but when _all_ the holders are released. The difference between blocking and non-blocking holders is only related to the busy-count value and when the worker is GCed/CCed. My patch fixes this but, it introduces an important issue with MozPromise.

Take a look at: https://searchfox.org/mozilla-central/source/xpcom/threads/MozPromise.h#461-466
Here, there is not a check of the return value of ::Dispatch(). But we should have a check, because, MozPromise doesn't use any WorkerHolder internally and the worker could be already shutting down when ::Dispatch() is called. The issue is easily reproducible with DOM Cache + workers, because DOM Cache is the only API using MozPromise exposed to workers (for now).

I'm still debugging this issue. I'll propose a solution soon.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

As we discussed in IRC, the patches in bug 1456466 may help.  Specifically P5, P6, and P8.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Andrea, now that bug 1457157 is fixed, can this move forward?

Comment 9

[Andrea Marchesini [:baku]]

Yes. I did some tests today. The leak is gone, but not immediately how the test wants. I'll rewrite it tomorrow using a ping/pong with BroadcastChannel or... with a timer.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Hey Andrea, what else has to happen for this bug to land?

Comment 11

[Andrea Marchesini [:baku]]

Pushed to try to see the state of these 2 patches. Keeping the NI.

Comment 12

[Eric Rahm [:erahm]]

Andrew, do you know who can pick this back up again?

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Putting this in the DWS_NEXT queue to be picked up.

Comment 14

[Eric Rahm [:erahm]]

Is DWS_NEXT still a thing?

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Yes, although this bug also benefits from being a P2.

Note for when we pick this up:

• Bug 1522246 potentially fixed the delay comment 9 was talking about.
• I could swear I saw some bug traffic go by recently about edge-cases relating to what/when DOMEventTargetHelper tries to help us drop things.