Re: testing event targets for memory leaks

[Ben Kelly]

On Thu, Apr 5, 2018 at 12:18 PM, Ben Kelly <bke...@mozilla.com> wrote:

> If one of these targets falls in your area, please try to find the time to
> write a small test as described above. Also, please link it against the
> meta bug here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1451787
>

Sorry to self-reply to an already long email, but...

Here are some hints to fix leaks in DOMEventTargetHelper (DETH) sub-classes:

1. Make sure you set the nsIGlobalObject owner by passing it to the DETH
constructor or by calling BindToOwner().
2. Override the DisconnectFromOwner() method to perform any cleanup. This
is now consistently called when the window is doomed.

Failing to do (1) caused MediaQueryList to leak (bug 1450271). We believe
failing to do (2) is causing the worker API to leak.

In the past code has had to use nsIObserver to listen for
inner-window-destroyed in order to do cleanup. This was due to
DisconnectFromOwner() not always getting called consistently. This should
now be fixed as of bug 1450266. It should now be possible to replace these
old listeners with a simpler DisconnectFromOwner() override.

[Ben Kelly]

Hi all,

I recently landed some test infrastructure for testing event targets for
memory leaks. This was part of fixing my service worker memory leak in bug
1447871. I wanted to let people know this existed and also ask for help
writing tests for more event targets.

To repeat, I need help writing more tests. Please see the list of untested
APIs at the bottom of this email.

To write an event target leak test you need to import this script:

<script type="text/javascript"
src="/tests/dom/events/test/event_leak_utils.js"></script>

And then call this function:

await checkForEventListenerLeaks("Worker", useWorker);

The string is just a name to make reporting assertions easier. The second
argument is a callback function where you exercise the API you want to test
for leaks. In this case I'm testing the worker API:

async function useWorker(contentWindow) {
contentWindow.messageCount = 0;
let w = new
contentWindow.Worker("data:application/javascript,self.postMessage({})");
w.onerror = _ => {
contentWindow.errorCount += 1;
};
await new Promise(resolve => {
w.onmessage = e => {
contentWindow.messageCount += 1;
resolve();
};
});
is(contentWindow.messageCount, 1, "message should be received");
}

The callback function should add an event listener that holds the given
contentWindow alive. Then try to leave the API in an active state to see
if its properly torn down when the test harness closes the window.

This particular test found that the Worker API leaks. See bug 1451381.

I've written tests for a number of APIs so far: ServiceWorker*, XHR,
Animation, MediaQueryList, EventSource, BroadcastChannel, MessageChannel,
SharedWorker, AbortSignal, WebSocket, Notification

The WebSocket and Notification tests have not landed yet because they
trigger assertions. These look like bugs in the respective APIs.

There are many, many event targets in the system, though. Using searchfox
to look for subclasses of DOMEventTargetHelper I came up with a list of
event targets that are not currently tested. Some of these are deprecated
or not exposed. But a lot of these are web exposed. It would be really
great to get this kind of simple leak test written for these APIs.

ContentFrameMessageManager
DOMRequest
ScreenOrientation
BatteryManager
OffscreenCanvas
ConstructibleEventTarget
FetchObserver
FileReader
IDBFileHandle
IDBMutableFileHandle
IDWrapperCache (and sub classes)
DOMMediaStream
MediaDevices
MediaRecorder
MediaStreamTrack
MediaTrack
MediaTrackList
TextTrack
TextTrackCue
TextTrackList
MediaKeySession
ImageCapture
MediaSource
SourceBuffer
SourceBufferList
AudioContext
AudioNode
SpeechRecognition
SpeechSynthesis
SpeechSynthesisUtterance
MIDIAccess
MIDIPort
Connection
TCPServerSocket
TCPSocket
UDPSocket
PaymentRequest
Performance
PermissionStatus
PresentationAvailability
PresentationConnection
PresentationConnectList
PresentationRequest
VRDisplay
FontFaceSet
ChannelWrapper
StreamFilter

If one of these targets falls in your area, please try to find the time to
write a small test as described above. Also, please link it against the
meta bug here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1451787

Thanks!

Ben

[Boris Zbarsky]

On 4/5/18 1:11 PM, Ben Kelly wrote:
> 1. Make sure you set the nsIGlobalObject owner by passing it to the DETH
> constructor or by calling BindToOwner().

Can we just enforce that by:

1) Removing the no-arg constructor from DETH.
2) Making the arg-taking constructors MOZ_ASSERT that the arg is not null.

? We'd need to fix some existing code, but after that should hopefully
be in a better place...

-Boris

[Randell Jesup]

>Hi all,
>
>I recently landed some test infrastructure for testing event targets for
>memory leaks. This was part of fixing my service worker memory leak in bug
>1447871. I wanted to let people know this existed and also ask for help
>writing tests for more event targets.
>
>To repeat, I need help writing more tests. Please see the list of untested
>APIs at the bottom of this email.

>There are many, many event targets in the system, though. Using searchfox
>to look for subclasses of DOMEventTargetHelper I came up with a list of
>event targets that are not currently tested. Some of these are deprecated
>or not exposed. But a lot of these are web exposed. It would be really
>great to get this kind of simple leak test written for these APIs.

I'm surprises that DOMDataChannel wasn't found:
nsDOMDataChannel.h:
class nsDOMDataChannel final : public mozilla::DOMEventTargetHelper,

perhaps you were looking for "public DOMEventTargetHelper"?

I also find nsScreen and nsDOMOfflineResourceList using
mozilla::DOMEventTargetHelper

Are there any events implemented in JS that we need to worry about? For
example, there are a lot of events (and a number of objects) defined for
WebRTC as part of dom/media/PeerConnection.js

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[Ben Kelly]

This would be nice to do yes, but was out of scope for the time I had. I
also suspect, though, that the non-binding DETH constructor is used for
objects created using the separate Init() method pattern. Not sure if its
fair to try to convert all of those.

Also keep in mind that binding to the global on a worker thread is somewhat
new. Many DETH objects only try to bind nsPIDOMWindowInner, but it would
be better to always bind nsIGlobalObject now.

Anyway, we should probably file a bug to consider it.

Ben

[Ben Kelly]

On Mon, Apr 9, 2018 at 12:32 PM, Ben Kelly <bke...@mozilla.com> wrote:

> On Mon, Apr 9, 2018 at 12:06 PM, Boris Zbarsky <bzba...@mit.edu> wrote:
>

> This would be nice to do yes, but was out of scope for the time I had. I
> also suspect, though, that the non-binding DETH constructor is used for
> objects created using the separate Init() method pattern. Not sure if its
> fair to try to convert all of those.
>
> Also keep in mind that binding to the global on a worker thread is
> somewhat new. Many DETH objects only try to bind nsPIDOMWindowInner, but
> it would be better to always bind nsIGlobalObject now.
>
> Anyway, we should probably file a bug to consider it.
>

Filed as:

https://bugzilla.mozilla.org/show_bug.cgi?id=1452705

[Ben Kelly]

On Mon, Apr 9, 2018 at 3:16 PM, Randell Jesup <rjesu...@jesup.org> wrote:

> I'm surprises that DOMDataChannel wasn't found:
> nsDOMDataChannel.h:
> class nsDOMDataChannel final : public mozilla::DOMEventTargetHelper,
>
> perhaps you were looking for "public DOMEventTargetHelper"?
>

Yep. That was indeed my lame search.

>
> I also find nsScreen and nsDOMOfflineResourceList using
> mozilla::DOMEventTargetHelper
>
> Are there any events implemented in JS that we need to worry about? For
> example, there are a lot of events (and a number of objects) defined for
> WebRTC as part of dom/media/PeerConnection.js
>

Having test coverage would be prudent, I think. I'm not sure what can
break in the js event target world.

Thanks.

Ben

[Ben Kelly]

PSA, I am landing additional leak tests for

AudioContext
IDB
WebSocket

As part of:

https://bugzilla.mozilla.org/show_bug.cgi?id=1451913

On Thu, Apr 5, 2018 at 12:18 PM, Ben Kelly <bke...@mozilla.com> wrote:

> Hi all,
>
> I recently landed some test infrastructure for testing event targets for
> memory leaks. This was part of fixing my service worker memory leak in bug
> 1447871. I wanted to let people know this existed and also ask for help
> writing tests for more event targets.
>
> To repeat, I need help writing more tests. Please see the list of
> untested APIs at the bottom of this email.
>

> There are many, many event targets in the system, though. Using searchfox
> to look for subclasses of DOMEventTargetHelper I came up with a list of
> event targets that are not currently tested. Some of these are deprecated
> or not exposed. But a lot of these are web exposed. It would be really
> great to get this kind of simple leak test written for these APIs.
>