Closed Bug 928389 Opened 7 years ago Closed 7 years ago

Default to strong listeners added via DOMRequestHelper

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

Details

Attachments

(2 files, 5 obsolete files)

Assignee: nobody → ferjmoreno
+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.
blocking-b2g: --- → koi?
(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?
Flags: needinfo?
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?
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.
Flags: needinfo?
Fabrice, I'll leave it up to you to decide if this needs to land in 1.2 or not.
Flags: needinfo?(fabrice)
I don't want to risk that on 1.2, especially with bug 924565 still in a bad state.
Flags: needinfo?(fabrice)
Thanks, Fabrice.  I'm going to clear the nom and Gene or others can re-nom to block a future release.
blocking-b2g: koi? → ---
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.
Yeah, I'll have a patch for this this week.
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+).
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()|?
(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
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.)
Attached patch Part 1: DOMRequestHelper (obsolete) — Splinter Review
Attached patch Part 2: Webapps (obsolete) — Splinter Review
Attachment #8336848 - Attachment is obsolete: true
Attachment #8336849 - Attachment is obsolete: true
Attachment #8336850 - Attachment is obsolete: true
Attached patch Part 1: DOMRequestHelper (obsolete) — Splinter Review
Attached patch Part 1: DOMRequestHelper (obsolete) — Splinter Review
Attachment #8336885 - Attachment is obsolete: true
(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.
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
Attachment #8336887 - Attachment is obsolete: true
Attachment #8340069 - Flags: review?(fabrice)
Attachment #8340069 - Flags: feedback?(gene.lian)
Attachment #8340069 - Flags: feedback?(bkelly)
Attachment #8336888 - Flags: review?(fabrice)
Comment on attachment 8340069 [details] [diff] [review]
Part 1: DOMRequestHelper

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

Looks good to me. Thank you. :)
Attachment #8340069 - Flags: feedback?(gene.lian) → feedback+
Attachment #8340069 - Flags: feedback?(bkelly) → feedback+
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.
blocking-b2g: --- → koi?
koi+ based on comment 25
blocking-b2g: koi? → koi+
Attachment #8336888 - Flags: review?(fabrice) → review+
Attachment #8340069 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/2f40cd447dc0
https://hg.mozilla.org/mozilla-central/rev/f3174f0757c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
No longer blocks: 944311
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.