Re: Is it necessary to remove message listeners?
Just to be clear though, if I find they are *not* all being removed, I should open a bug on that rather than just removing the listeners myself and calling it done? ie, is it accurate to say that it *should* not be necessary to remove these handlers (and, if I verify that is true, that I could explicitly add a note to this effect on the relevant MDN pages?) You'd have to ask smaug to be sure, but judging from what I looked at, I'd think that we are trying to remove the event listeners when the relevant window and/or goes away. So I'd file a bug; we can always resolve it as invalid. [1] https://bug893242.bugzilla.mozilla.org/attachment.cgi?id=774978 On Thu, Jul 25, 2013 at 6:51 PM, Mark Hammond mhamm...@skippinet.com.au wrote: Felipe and I were having a discussion around a patch that uses nsIMessageManager. Specifically, we create a browser element, then call browser.messageManager.addMessageListener() with the requirement that the listener live for as long as the browser element itself. The question we had was whether it was necessary to explicitly call removeMessageListener, or whether we can rely on automatic cleanup when the browser element dies? It seems obvious to us that it *should* be safe to rely on automatic cleanup, but searching both docs and mxr didn't make it clear, so I figured it was better to ask rather than to cargo-cult the addition of explicit cleanup code that wasn't necessary. Thanks, Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Is it necessary to remove message listeners?
On 07/27/2013 06:06 AM, Mark Hammond wrote: On 27/07/2013 2:53 AM, Justin Lebar wrote: ... Whether or not we totally succeed in this endeavor is another question entirely. You could instrument your build to count the number of live nsFrameMessageManager objects and report the number of message listeners in each -- one way to do this would be with a patch very similar to this one [1]. Thanks for following up. I'll add some hacks to count the message listeners as you suggest and followup here with what I find. Just to be clear though, if I find they are *not* all being removed, I should open a bug on that rather than just removing the listeners myself and calling it done? ie, is it accurate to say that it *should* not be necessary to remove these handlers (and, if I verify that is true, that I could explicitly add a note to this effect on the relevant MDN pages?) Yes, one shouldn't have to remove message listeners. Message listeners should in most cases work like event listeners. But, as with event listeners, remember that the callback may keep stuff alive. So after you have handled the message/event you're expecting, and don't need to handle more such messages/events, you perhaps want to remove the listener manually. Thanks, Mark [1] https://bug893242.bugzilla.mozilla.org/attachment.cgi?id=774978 On Thu, Jul 25, 2013 at 6:51 PM, Mark Hammond mhamm...@skippinet.com.au wrote: Felipe and I were having a discussion around a patch that uses nsIMessageManager. Specifically, we create a browser element, then call browser.messageManager.addMessageListener() with the requirement that the listener live for as long as the browser element itself. The question we had was whether it was necessary to explicitly call removeMessageListener, or whether we can rely on automatic cleanup when the browser element dies? It seems obvious to us that it *should* be safe to rely on automatic cleanup, but searching both docs and mxr didn't make it clear, so I figured it was better to ask rather than to cargo-cult the addition of explicit cleanup code that wasn't necessary. Thanks, Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Is it necessary to remove message listeners?
Thanks for asking about this; we have a lot of unnecessary unlinking code in our JS, Let me share how I investigated your question. $ git grep -i addmessagelistener -- '*.cpp' content/base/src/nsFrameMessageManager.cpp:nsFrameMessageManager::AddMessageListener(const nsAString aMessage, Only one hit; that's easy. Looking at the function, message listeners are stored in nsFrameMessageManager::mListeners. So what's the lifetime of that array? The array is cleared in a few places: 1) nsFrameMessageManager::Disconnect(), 2) MMListenerRemover::~MMListenerRemover 3) nsFrameMessageManager's CC unlink function, It looks like (2) is just a way of delaying Disconnect()'s removal until message handling is complete. So when do we call Disconnect()? http://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3AnsFrameMessageManager%3A%3ADisconnect%28_Bool%29 At this point it's clear that we /attempt/ to clear the message listeners when the message manager's window goes away. That's probably as far as you'd get by asking someone. Whether or not we totally succeed in this endeavor is another question entirely. You could instrument your build to count the number of live nsFrameMessageManager objects and report the number of message listeners in each -- one way to do this would be with a patch very similar to this one [1]. [1] https://bug893242.bugzilla.mozilla.org/attachment.cgi?id=774978 On Thu, Jul 25, 2013 at 6:51 PM, Mark Hammond mhamm...@skippinet.com.au wrote: Felipe and I were having a discussion around a patch that uses nsIMessageManager. Specifically, we create a browser element, then call browser.messageManager.addMessageListener() with the requirement that the listener live for as long as the browser element itself. The question we had was whether it was necessary to explicitly call removeMessageListener, or whether we can rely on automatic cleanup when the browser element dies? It seems obvious to us that it *should* be safe to rely on automatic cleanup, but searching both docs and mxr didn't make it clear, so I figured it was better to ask rather than to cargo-cult the addition of explicit cleanup code that wasn't necessary. Thanks, Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Is it necessary to remove message listeners?
On 27/07/2013 2:53 AM, Justin Lebar wrote: ... Whether or not we totally succeed in this endeavor is another question entirely. You could instrument your build to count the number of live nsFrameMessageManager objects and report the number of message listeners in each -- one way to do this would be with a patch very similar to this one [1]. Thanks for following up. I'll add some hacks to count the message listeners as you suggest and followup here with what I find. Just to be clear though, if I find they are *not* all being removed, I should open a bug on that rather than just removing the listeners myself and calling it done? ie, is it accurate to say that it *should* not be necessary to remove these handlers (and, if I verify that is true, that I could explicitly add a note to this effect on the relevant MDN pages?) Thanks, Mark [1] https://bug893242.bugzilla.mozilla.org/attachment.cgi?id=774978 On Thu, Jul 25, 2013 at 6:51 PM, Mark Hammond mhamm...@skippinet.com.au wrote: Felipe and I were having a discussion around a patch that uses nsIMessageManager. Specifically, we create a browser element, then call browser.messageManager.addMessageListener() with the requirement that the listener live for as long as the browser element itself. The question we had was whether it was necessary to explicitly call removeMessageListener, or whether we can rely on automatic cleanup when the browser element dies? It seems obvious to us that it *should* be safe to rely on automatic cleanup, but searching both docs and mxr didn't make it clear, so I figured it was better to ask rather than to cargo-cult the addition of explicit cleanup code that wasn't necessary. Thanks, Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform