Re: testing event targets for memory leaks
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. > > To write an event target leak test you need to import this 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 > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
On Mon, Apr 9, 2018 at 3:16 PM, Randell Jesupwrote: > 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
>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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
On Mon, Apr 9, 2018 at 12:32 PM, Ben Kellywrote: > On Mon, Apr 9, 2018 at 12:06 PM, Boris Zbarsky wrote: > >> 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... >> > > 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
On Mon, Apr 9, 2018 at 12:06 PM, Boris Zbarskywrote: > 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... > 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: testing event targets for memory leaks
On Thu, Apr 5, 2018 at 12:18 PM, Ben Kellywrote: > 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. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
testing event targets for memory leaks
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: 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform