On 09/17/2013 02:07 PM, Fernando Jiménez Moreno wrote:
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.
I'm not too familiar with DOMRequestHelper, but adding listeners only if they
are actually needed sounds very sane thing to do :)
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).
Well, not "as soon as", but probably when GC and CC have run.
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.
Yeah, such listeners would need to be kept alive in some other way.
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.
Well, weak message listeners are a new thing anyway, so it is not very
surprising it takes some time to find the right way utilize them.
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
We need to still make sure we then remove those listeners at some point, unless it is ok to keep the objects alive as long as the message manager is
alive.
, 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.
....or keep the listeners alive by some other mechanism. Like adding some
expando property to the relevant window and keep listeners alive there.
khueyfix should then kill those references when the window goes away, right
khuey?
-Olli
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
