698570 - Use weak references in nsFormHistory.js

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use weak ownership

https://tbpl.mozilla.org/?tree=Try&rev=080f85d077d5

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use weak references in prefs in nsFormHistory.js

Looks like I have to switch to using the observer method first. A bit obvious now that I noticed it.

While I code that, this patch changes the pref observer to be weak.

https://tbpl.mozilla.org/?tree=Try&rev=3c4098ee41ae

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use weak references in prefs in nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=e299b84494cf

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use weak references in prefs in nsFormHistory.js

https://hg.mozilla.org/try/rev/5c03b56abe52

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

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

the relation of this patch with bug 698738 is unclear, one of the two should actually convert observers to weak references... none of them is doing

Comment 6

[Marco Bonardo [:mak]]

unless you plan to do that in bug 696404?

Comment 7

[Honza Bambas (:mayhemer)]

This patch and bug jungle is over my understanding too, I can't keep track on this any more.  Patches and parts are seem jumping here and there, comments are spread across.  This is case where less is apparently more :)  

This should merge to a singe or up to two patches with a clear distinction, but it is probably too late.  

Rafael, thanks for your work on this, but please set the bug dependencies properly and/or give us a guide to keep track please.  Thank you.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

Yes, this is the way to let js components be weak-reffed.

There are other addObserver calls that need to be converted too then.  I've seen changes to nsFormHistory.js also in a different bug, no idea which one it is and how exactly is related to this one.

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I had bad experiences changing code in this area. It is really easy to produce failures that can take one down in a long debug path. The review process also tends to add "there is something else wrong on that file, could you fix it too?", which is particularly annoying when that part the most controversial one as was the case with the "add the missing remeoveObserver/ use weak links".

That is why I am making the patches as small as possible. This is one is *just* about using weak references in nsFormHistory.js. That is one of the extra request of an old review, I assume it is a good and independent fix, no?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

bug 698738 should land first, then this should convert all the observers in nsFormHistory.js to weak references. Otherwise we are going to lose pieces along the way.
So, once that lands, update this patch and reask for review, please.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Marco Bonardo [:mak] from comment #10)
> So, once that lands, update this patch and reask for review, please.

Marco, thank for adding at least some light to dependencies.

Comment 12

[Marco Bonardo [:mak]]

As an additional comment, I think that "let self = this;" is now unused and can be removed. Should have been done in bug 698738 but I missed that, so please do here.

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use weak references in nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=e22826384f38

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 573237 [details] [diff] [review]
Use weak references in nsFormHistory.js

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

thanks!

Comment 15

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/96123bd11904

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/96123bd11904