928389 - Default to strong listeners added via DOMRequestHelper

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

https://bugzilla.mozilla.org/show_bug.cgi?id=927465#c3

Comment 1

[Gene Lian (I already quit Mozilla)]

+1.

I have a strong feeling that adding listeners as weak ones by default is very dangerous when Justin changed everything to be weak referenced to the window in the first place. IMO, the default value should be strong referenced unless it needs to be added as weak referenced (ex. for the application object). Otherwise, it would potentially cause random failure when doing cpmm/ppmm messaging, which is very hard to discover and reproduce.

Nominating for koi+ although I'm not sure which PM knows better about the context of this bug.

Comment 2

[Andrew Overholt [:overholt]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #1)
> +1.
> 
> I have a strong feeling that adding listeners as weak ones by default is
> very dangerous when Justin changed everything to be weak referenced to the
> window in the first place. IMO, the default value should be strong
> referenced unless it needs to be added as weak referenced (ex. for the
> application object). Otherwise, it would potentially cause random failure
> when doing cpmm/ppmm messaging, which is very hard to discover and reproduce.

Potential random failures are bad but I'm not sure we'd hold the 1.2 release for fixing this.

> Nominating for koi+

There are some rough guidelines here:

  https://wiki.mozilla.org/B2G/Triage#Issues_that_Should_Block

Do you think we should hold 1.2 for this?

Comment 3

[Gene Lian (I already quit Mozilla)]

All the DOMRequestHelper related issues have been checked in into v1.2, so I'm just wondering we should land this as well. However, it seems not yet causing any regression or critical bugs so far, so I don't have enough evidence to make this bug a blocking one, though. Maybe solving it on V1.3 is OK (V1.2 doesn't have vendors anyway).

Fabrice and Fernando, what do you think?

Comment 4

[[:fabrice] Fabrice Desré]

We actually have a regression in bug 924565 that may need us to backout bug 915598. So hold on doing anything here for now. Also, 1.2 may not have new OEMs, but there are new partners and updates.

Comment 5

[Andrew Overholt [:overholt]]

Fabrice, I'll leave it up to you to decide if this needs to land in 1.2 or not.

Comment 6

[[:fabrice] Fabrice Desré]

I don't want to risk that on 1.2, especially with bug 924565 still in a bad state.

Comment 7

[Andrew Overholt [:overholt]]

Thanks, Fabrice.  I'm going to clear the nom and Gene or others can re-nom to block a future release.

Comment 8

[Gene Lian (I already quit Mozilla)]

Ping Fernando? :) Hopefully, the recent DOMRequestHelper change doesn't beat you. ^^a

I'm still more comfortable to add strong listeners as default via DOMRequestHelper. We still ran into Bug 937374, which sounds the unstableness of weak listeners.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Yeah, I'll have a patch for this this week.

Comment 10

[Gene Lian (I already quit Mozilla)]

Thanks Fernando!

After bug 924565 lands, all the objects that are supposed to be added as weak references can now be correctly added as weak ones (because the observer "inner-window-destroyed" is now correctly added as a weak reference). This fix would potentially cause other bugs for those objects that should be added as strong references (i.e. those ones were originally working is due to the mistake of adding "inner-window-destroyed" as strong).

Fixing this bug can solve the above-mentioned issue but is seems we'd better land this to koi+ as well (because bug 924565 is also koi+).

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Is it that we want these objects to stick around as long as there are outstanding requests?  Would it be enough to add a strong reference in |DOMRequestHelper.getRequestId()| and then remove the strong ref in |DOMRequestHelper.removeRequest()|?

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ben Kelly [:bkelly] from comment #11)
> Is it that we want these objects to stick around as long as there are
> outstanding requests?  Would it be enough to add a strong reference in
> |DOMRequestHelper.getRequestId()| and then remove the strong ref in
> |DOMRequestHelper.removeRequest()|?

IMHO that's something that should be delegated to objects inheriting from DOMRequestIPCHelper. Releasing a request doesn't need to be associated with removing message listeners. There are cases where IPC messages are not associated with DOMRequests. Check IACMessagePort implementation for example [1]. Or cases where we don't want to remove the listeners after getting the first message.

The main reason for adding "(add/remove)MessageListener" in bug 915598 was basically to allow objects that inherit from DOMRequestIPCHelper to handle its specific needs.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppMessagePort.js

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I guess it just feels error prone to debug each child of DOMRequestIPCHelper individually.  It would be nice if we could identify common use cases and automatically DTRT.

The two general cases I can think of are:

1) Request/response communication over IPC where we want to ensure the object stays alive to receive the response.
2) Async events like mozContacts oncontactchange where the object needs to stick around as long as there is an event handler registered from the client.

Are there more uses cases than these?

(Incidentally, I think that mozContacts oncontactchange event is probably effected by weak ref cleanup at the moment.  It should wait to register the "Contact:Changed" event until onconctactchange() is called and then use a strong ref.)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOMRequestHelper

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Webapps

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Remove nsISupportsWeakReference where not needed

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOMRequestHelper

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOMRequestHelper

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Webapps

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=3039e846dfa2

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ben Kelly [:bkelly] from comment #13)
> I guess it just feels error prone to debug each child of DOMRequestIPCHelper
> individually.  It would be nice if we could identify common use cases and
> automatically DTRT.
> 
> The two general cases I can think of are:
> 
> 1) Request/response communication over IPC where we want to ensure the
> object stays alive to receive the response.
> 2) Async events like mozContacts oncontactchange where the object needs to
> stick around as long as there is an event handler registered from the client.
> 
> Are there more uses cases than these?
> 
> (Incidentally, I think that mozContacts oncontactchange event is probably
> effected by weak ref cleanup at the moment.  It should wait to register the
> "Contact:Changed" event until onconctactchange() is called and then use a
> strong ref.)

We talked about this over IRC and we agreed to focus on defaulting to strong refs first and maybe add some helper routines for common cases as follow ups.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Try build looks good.

B2GPerf also looks good:

- default to weakRef:

2013-11-28 17:04:25 | B2GPerfRunner | INFO | Results for Browser, cold_load_time: median:550, mean:579, std: 59, max:756, min:537, all:756,559,672,556,562,539,537,537,599,548,576,542,543,541,597,546,599,543,550,541,693,553,547,547,710,551,549,542,688,551


- default to stronRef:

2013-11-28 17:27:10 | B2GPerfRunner | INFO | Results for Browser, cold_load_time: median:548, mean:573, std: 47, max:714, min:538, all:688,579,574,539,548,542,634,543,573,544,545,538,612,542,580,545,544,539,657,549,578,548,546,543,714,551,548,558,550,654

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOMRequestHelper

Comment 24

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8340069 [details] [diff] [review]
Part 1: DOMRequestHelper

Review of attachment 8340069 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thank you. :)

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Note that we have had a report of the contacts change events getting lost which is probably a symptom of the default weak references.  We probably need to uplift this to b2g26_v1_2 or back out bug 924565 there.  The advantage of uplifting over backing out is we would be able to avoid the leak of the app objects.

Comment 26

[Preeti Raghunath(:Preeti)]

koi+ based on comment 25

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f40cd447dc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3174f0757c8

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f40cd447dc0
https://hg.mozilla.org/mozilla-central/rev/f3174f0757c8

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9a7461bf4b29
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/53a99f2b6596