Hi folks! while working on [1] and [2] I had a few doubts and I found out that we might have a couple of issues with DOMRequestHelper:
1. == We are adding message listeners for messages that might never happen. == This is because the only way to add message listeners via DOMRequestHelper is during the window creation [3] and [4]. In most of the objects inheriting [5] from DOMRequestHelper we are adding listeners for messages that are associated *only* with an specific request and we are doing this within "nsIDOMGlobalPropertyInitializer.init()". For example, for the Contacts API, we are adding a bunch of "*:Return:OK" and "*:Return:KO" messages at [6], but we might only use a few of them, or even not use them at all. These messages are expected to be received as responses for a known request. For example, the ".find" [7] request sends a "Contacts:Find" message that expects either a "Contacts:Find:Return:OK" or a "Contacts:Find:Return:KO" reply. IMHO we should be adding these message listeners *only* when we really expect to receive them. That's it, when we create the DOMRequest associated with these messages. I might not be understanding the reason for adding these listeners with "initDOMRequestHelper" instead of with each specific request, but I would like to propose adding the message listeners *only* when they are actually needed. I would like to change "DOMRequestHelper.createRequest" [9] to accept a list of messages to listen for. Note that I still want to use DOMRequestHelper to add these message listeners, so we can keep track of them in order to remove them when the window is destroyed [10], if they are still around. This change would affect quite a few APIs, so I'd like to hear your feedback before starting to work on it. 2. == As bug [2] says, we need strong references in some cases == I am pretty far from being an expert on these topics (so any help is highly appreciated :) ), but for what I can understand, by adding a weak message listener at [10], the message manager keeps a weak reference to the listener, which in this case is an object inheriting from DOMRequestHelper. This relies on the object listening for messages to not go out of scope. As soon as this object is not used in the content side, the message manager will remove the listener [11] (Again, please correct me if I am wrong with my assumptions). This is great for the message listeners associated with an specific request, as the ones that I mentioned above, but I am afraid that it wouldn't work well for listeners that needs to be alive during the lifetime of the window. There are several cases where we want this. For example, we need the "Webapps:Install:Return:OK" (or *KO) message to be alive. Let's say an app A (Gaia System app for example) with mozApps.mgmt permissions adds "Webapps:Install :Return:*" message listeners during its window init process, but these listeners are quickly removed as soon as the webapps mgmt object goes out of scope. After that an app B calls mozApps.install() and the install process goes ok. This will eventually broadcast a "Webapps:Install:Return:OK" message [12], that will never be received by app A, cause its listener went out of scope and was removed. I would really appreciate if you could confirm that the above description is correct (even if it is not expected). As I said, I am not really familiar with all of this tricky stuff. My proposal to change this is to add strong referenced message listeners in these cases. I would say that we could add strong references by default for all message listeners added via initDOMRequestHelper() during the window init process, if we finally do what I proposed in 1. Summary: - We need to add message listeners only when they are actually needed. - For "one-shot" messages associated with specific requests, we should add weak message listeners. - For messages that should be listened during the lifetime of the window, we should add strong message listeners. Cheers! / Fernando [1] https://bugzilla.mozilla.org/show_bug.cgi?id=915898 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=915598 [3] https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm#116 [4] https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm#42 [5] https://mxr.mozilla.org/mozilla-central/search?string=DOMRequestIpcHelper.prototype [6] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#941 [7] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#827 [8] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#822 [9] https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm#46 [10] https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm#43 [11] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#762 [12] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2210 _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
