Closed Bug 767682 Opened 8 years ago Closed 5 years ago

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

Categories

(SeaMonkey :: General, defect, P2, major)

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.13

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
Depends on: 750454
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.
Attachment #637020 - Flags: review?(neil)
Simple port, and more nits.
(Untested.)
Attachment #637022 - Flags: review?(neil)
Nits, plus 2 fixes:
*s/getService[Ci.mozIAsyncLivemarks]/getService(Ci.mozIAsyncLivemarks)/
*Add missing |delete this.bookmarks;|, which correctly fixes bug 758459.

(Untested.)
Attachment #637024 - Flags: review?(mak77)
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, sgautherie.bz@free.fr-67dbc42bd72d/try-comm-central-win32)

suite/smile/test/ m-b-c still pass with the 4 SM patches applied :-)
(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 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.)
Attachment #637019 - Flags: review?(neil) → review+
Attachment #637020 - Flags: review?(neil) → review+
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.
Attachment #637021 - Flags: review?(neil) → review+
Attachment #637022 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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 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?
(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.
(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 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
sgautherie.bz@free.fr – Thu Jun 28 06:56:06 2012 PDT
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).
Attachment #637019 - Attachment description: (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey → (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey [Checked in: Comment 15]
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
Attachment #637020 - Attachment description: (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey → (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey [Checked in: Comment 16]
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
Attachment #637021 - Attachment description: (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey → (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey [Checked in: Comment 17]
Depends on: 770477
(In reply to neil@parkwaycc.co.uk from comment #12)
> As far as I can tell, this code is useless in FUEL too.

Bug 770477 Submitted
(In reply to neil@parkwaycc.co.uk 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?
Attachment #639032 - Flags: review?(neil)
(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.
Attachment #639032 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk 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?
> Then shouldn't we expand the few Ci which are in our file?
Yes. but this is low priority.
(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 :-(
(In reply to neil@parkwaycc.co.uk 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 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
Attachment #639032 - Attachment description: (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| (whitespace and function name) to SeaMonkey → (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Checked in: Comment 25]
Attachment #639617 - Flags: review?(neil)
Attachment #637022 - Attachment description: (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey → (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Split into Ev1 and Fv1]
Attachment #637022 - Attachment is obsolete: true
Attachment #637024 - Attachment description: (Ev1-FF) Resync/Fix fuelApplication.js from smileApplication.js → (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js
Attachment #637024 - Attachment filename: 767682-Ev1-FF_resync-fix.diff → 767682-Gv1-FF_resync-fix.diff
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]
Attachment #639617 - Flags: review?(neil) → review+
Fv1, with comment 27 suggestion(s).
Attachment #639617 - Attachment is obsolete: true
Attachment #640199 - Flags: review?(neil)
Comment on attachment 640199 [details] [diff] [review]
(Fv1a) smileApplication.js: Use Services.*, Fix a few nits

Where does Services.jsm get imported?
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
]),
Attachment #637024 - Flags: review?(mak77) → feedback+
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.
Attachment #640199 - Flags: review?(neil)
>  (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js
Please file a new bug in Firefox. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.