767682 - Port |Bug 750454 - FUEL causes lots of leaks until shutdown, can also cause 10+minute shutdown times| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey [Checked in: Comment 15]

Simple port.
(Untested.)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey [Checked in: Comment 16]

Simple port.
(Untested.)

NB:
I'm not sure whether |.bind(this)| could still be used, so I simply copied |var self = this;|.
I used |Ci|, which is explicitly defined in this file.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey [Checked in: Comment 17]

Trivial copy.
(Untested.)

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Split into Ev1 and Fv1]

Simple port, and more nits.
(Untested.)

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js

Nits, plus 2 fixes:
*s/getService[Ci.mozIAsyncLivemarks]/getService(Ci.mozIAsyncLivemarks)/
*Add missing |delete this.bookmarks;|, which correctly fixes bug 758459.

(Untested.)

Comment 6

[Serge Gautherie (:sgautherie)]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=37e2a400b4d9
[Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0 SeaMonkey/2.13a1 // Build identifier: 20120627041654] (MsVc8, [e-mail]-67dbc42bd72d/try-comm-central-win32)

suite/smile/test/ m-b-c still pass with the 4 SM patches applied :-)

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #6)
> suite/smile/test/ m-b-c still pass with the 4 SM patches applied :-)

(on local Windows XP)

Comment 8

[[e-mail]]

Comment on attachment 637019 [details] [diff] [review]
(Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey
[Checked in: Comment 15]

>+    this._tabbrowser.addEventListener(aType, this,
>+                                      /* useCapture = */ true);
Please can you remove the useCapture comments. Anyone who wants to patch this code had better know what the third argument means ;-)

>+  } else {
>+    // This tab may have moved to another window, so make sure its cached
>+    // window is up-to-date.
>+    smileBrowserTab._window = aSMILEWindow;
Actually I don't think this is possible. Even if an extension "moves" a tab to a new window, it would need to create new <tab> and <browser> elements in the new window's document, so that a <tab> element could never exist in more than one window. r=me with this removed. (Ensure that getWindow and getBrowserTab use the same formatting.)

Comment 9

[[e-mail]]

Comment on attachment 637021 [details] [diff] [review]
(Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey
[Checked in: Comment 17]

r=me assuming the tests pass.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #8)

> Please can you remove the useCapture comments. Anyone who wants to patch
> this code had better know what the third argument means ;-)

This was discussed in bug 750454 comment 60.
I would rather just copy FF code, but I won't insist if you really don't want it.

> >+  } else {
> >+    // This tab may have moved to another window, so make sure its cached
> >+    // window is up-to-date.
> >+    smileBrowserTab._window = aSMILEWindow;
> Actually I don't think this is possible. Even if an extension "moves" a tab
> to a new window, it would need to create new <tab> and <browser> elements in
> the new window's document, so that a <tab> element could never exist in more
> than one window. r=me with this removed. (Ensure that getWindow and
> getBrowserTab use the same formatting.)

Is this a difference between SMILE and FUEL, or would you say this code is useless in FUEL too?
(See bug 750454 comment 53.)

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 637022 [details] [diff] [review]
(Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey
[Split into Ev1 and Fv1]

Did you mean to r+, or what is wrong with this patch?

Comment 12

[[e-mail]]

(In reply to Serge Gautherie from comment #10)
> (In reply to comment #8)
> > Please can you remove the useCapture comments.
> This was discussed in bug 750454 comment 60.
No, that was ownsWeak comments; you can leave those in if you insist.

> > >+  } else {
> > >+    // This tab may have moved to another window, so make sure its cached
> > >+    // window is up-to-date.
> > >+    smileBrowserTab._window = aSMILEWindow;
> > Actually I don't think this is possible. Even if an extension "moves" a tab
> > to a new window, it would need to create new <tab> and <browser> elements in
> > the new window's document, so that a <tab> element could never exist in more
> > than one window. r=me with this removed. (Ensure that getWindow and
> > getBrowserTab use the same formatting.)
> Is this a difference between SMILE and FUEL, or would you say this code is
> useless in FUEL too?
As far as I can tell, this code is useless in FUEL too.

Comment 13

[[e-mail]]

(In reply to Serge Gautherie from comment #11)
> Did you mean to r+, or what is wrong with this patch?
I don't like the "reformatting". The whitespace changes are OK though.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 637024 [details] [diff] [review]
(Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js

mochitest-other succeeded as
https://tbpl.mozilla.org/?tree=Try&rev=9c3b516c25e6
[e-mail] – Thu Jun 28 06:56:06 2012 PDT

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 637019 [details] [diff] [review]
(Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey
[Checked in: Comment 15]

http://hg.mozilla.org/comm-central/rev/88b36d0d1a85
Av1, with comment 8 suggestion(s).

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 637020 [details] [diff] [review]
(Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey
[Checked in: Comment 16]

http://hg.mozilla.org/comm-central/rev/4e50453f250d

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 637021 [details] [diff] [review]
(Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/d61f5bc92f00

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #12)
> As far as I can tell, this code is useless in FUEL too.

Bug 770477 Submitted

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Checked in: Comment 25]

(In reply to [e-mail] from comment #13)
> I don't like the "reformatting". The whitespace changes are OK though.

This file already uses Ci in some places: why not use Cc/Ci everywhere?

Comment 20

[[e-mail]]

(In reply to Serge Gautherie from comment #19)
> This file already uses Ci in some places
Only because extApplication.js needs it and doesn't define it itself.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #20)
> (In reply to Serge Gautherie from comment #19)
> > This file already uses Ci in some places
> Only because extApplication.js needs it and doesn't define it itself.

Then shouldn't we expand the few Ci which are in our file?

Comment 22

[Philip Chee]

> Then shouldn't we expand the few Ci which are in our file?
Yes. but this is low priority.

Comment 23

[[e-mail]]

(In reply to Serge Gautherie from comment #19)
> This file already uses Ci in some places
Only because I only just noticed them in your earlier attachments :-(

Comment 24

[[e-mail]]

(In reply to [e-mail] from comment #23)
> (In reply to Serge Gautherie from comment #19)
> > This file already uses Ci in some places
> Only because I only just noticed them in your earlier attachment :-(
Attachment 637020 [details] [diff] to be precise.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 639032 [details] [diff] [review]
(Ev1) Port |Bug 750454 Part 8: Reformat FUEL code|  to SeaMonkey
[Checked in: Comment 25]

http://hg.mozilla.org/comm-central/rev/7b0299689777

Comment 26

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) smileApplication.js: Fix a few nits

Comment 27

[[e-mail]]

Comment on attachment 639617 [details] [diff] [review]
(Fv1) smileApplication.js: Fix a few nits

>+    return Components.classes["@mozilla.org/network/io-service;1"]
>+                     .getService(Components.interfaces.nsIIOService)
>+                     .newURI(aSpec, null, null);
[I guess we'll switch to Services.io eventually]

>+    for (var i = 0; i < tabNodes.length; ++i)
[Not terribly keen on ++i]

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1a) smileApplication.js: Use Services.*, Fix a few nits

Fv1, with comment 27 suggestion(s).

Comment 29

[[e-mail]]

Comment on attachment 640199 [details] [diff] [review]
(Fv1a) smileApplication.js: Use Services.*, Fix a few nits

Where does Services.jsm get imported?

Comment 30

[Marco Bonardo [:mak]]

Comment on attachment 637024 [details] [diff] [review]
(Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js

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

Looks good apart a couple things

::: browser/fuel/src/fuelApplication.js
@@ +14,5 @@
>  // Singleton that holds services and utilities
>  var Utilities = {
>    get bookmarks() {
>      let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
> +                     getService(Ci.nsINavBookmarksService);

I honestly don't understand these indentation changes, we always indented as

Cc[
getService

or

Cc[
  .getService

never seen it indented between the 2 "c"

@@ +150,5 @@
>    let fuelBrowserTab = fuelBrowserTabMap.get(aBrowser);
>    if (!fuelBrowserTab) {
>      fuelBrowserTab = new BrowserTab(aFUELWindow, aBrowser);
>      fuelBrowserTabMap.set(aBrowser, fuelBrowserTab);
> +  } else {

all the file uses uncuddled else, so while cuddled is coherent with the coding guide, is not coherent with this specific file.

@@ +553,3 @@
>            Utilities.bookmarksObserver.addListener(self._id, aEvent, aListener);
>          }
> +      } else {

ditto

@@ +565,3 @@
>            Utilities.bookmarksObserver.removeListener(self._id, aEvent, aListener);
>          }
> +      } else {

ditto

@@ +767,5 @@
> +  QueryInterface: XPCOMUtils.generateQI(
> +                    [Ci.fuelIApplication,
> +                     Ci.extIApplication,
> +                     Ci.nsIObserver,
> +                     Ci.nsISupportsWeakReference]),

nit: I'd rather suggest
QueryInterface: XPCOMUtils.generateQI([
  Ci.xxx
]),

Comment 31

[Philip Chee]

Comment on attachment 640199 [details] [diff] [review]
(Fv1a) smileApplication.js: Use Services.*, Fix a few nits

> (Fv1a) smileApplication.js: Use Services.*, Fix a few nits
This should really go into a new bug. This one is long enough.

Comment 32

[Philip Chee]

>  (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js
Please file a new bug in Firefox. Thanks.