Closed Bug 725529 Opened 8 years ago Closed 8 years ago

Fix 4 "leaked window property" in SeaMonkey

Categories

(SeaMonkey :: Testing Infrastructure, defect, P3, major)

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.10

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328720232.1328724545.2902.gz
OS X 10.5 comm-central-trunk debug test mochitest-other on 2012/02/08 08:57:12
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328740940.1328745964.22504.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/08 14:42:20
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_library_infoBox.js | leaked window property: childNode

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js | leaked window property: filePath1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_346337.js | leaked window property: filePath2

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/modules/test/browser_sanitizer.js | leaked window property: Sanitizer
}
Depends on: 557496
Port
http://hg.mozilla.org/mozilla-central/rev/f92194f05828
"No bug - fix some browser chrome tests that pollute the global scope"
Attachment #595744 - Flags: review?(bugspam.Callek)
I found these while working on patch Bv1:
as a confirmation, notice that they are not affected by this very bug ;-)

http://mxr.mozilla.org/comm-central/search?string=.import(&case=1&find=%2Fsuite%2F.*%2Ftest.*%2Fbrowser

NB: This is also related to my bug 707786 comment 10...
Attachment #595774 - Flags: review?(neil)
Comment on attachment 595736 [details] [diff] [review]
(Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope
[Checked in: See comment 7]

Jens, feel free to take over other reviews if that applies...
Attachment #595736 - Flags: review?(jh)
Comment on attachment 595736 [details] [diff] [review]
(Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope
[Checked in: See comment 7]

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

::: suite/modules/test/browser_sanitizer.js
@@ +4,1 @@
>  ok(typeof Sanitizer != "undefined", "Sanitizer module imported")

While you're here, please add a semicolon. :-)

r=me with that.
Attachment #595736 - Flags: review?(jh)
Attachment #595736 - Flags: review?(bugspam.Callek)
Attachment #595736 - Flags: review+
Comment on attachment 595736 [details] [diff] [review]
(Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope
[Checked in: See comment 7]

http://hg.mozilla.org/comm-central/rev/185700094ffb
Bv1, with comment 6 suggestion(s).
Attachment #595736 - Attachment description: (Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope → (Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope [Checked in: See comment 7]
Comment on attachment 595744 [details] [diff] [review]
(Cv1) browser_346337.js: Add 2 missing |var|
[Checked in: See comment 11]

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

[You could just as well have used "let" here I guess.]
Attachment #595744 - Flags: review?(bugspam.Callek) → review+
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/suite/common/tests/browser/
> browser_346337.js | Test timed out

Is this, which I reproducibly get even with Cv1 applied, addressed somewhere (here or elsewhere)?
Comment on attachment 595612 [details] [diff] [review]
(Av1) browser_library_infoBox.js: Add a missing |var childNode;|
[Checked in: See comment 18]

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

::: suite/common/places/tests/browser/browser_library_infoBox.js
@@ +68,5 @@
>         "Expander button is hidden for all bookmarks node.");
>      checkAddInfoFieldsCollapsed(PO);
>  
> +    // Note: SeaMonkey handles bookmarks only, not history.
> +    var childNode;

This is a very odd place to define childNode. Why not just change the existing line (below what's visible here)?

And what is the comment supposed to mean? Is it actually necessary? What is the context (code wise; I guess it's generally referring to the Bookmarks Manager)?
Comment on attachment 595744 [details] [diff] [review]
(Cv1) browser_346337.js: Add 2 missing |var|
[Checked in: See comment 11]

http://hg.mozilla.org/comm-central/rev/4323497b9d80
Cv1, with comment 8 suggestion(s).


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #9)
> Is this, which I reproducibly get even with Cv1 applied, addressed somewhere
> (here or elsewhere)?

Bug 725679 ;-)
Attachment #595744 - Attachment description: (Cv1) browser_346337.js: Add 2 missing |var| → (Cv1) browser_346337.js: Add 2 missing |var| [Checked in: See comment 11]
Comment on attachment 595612 [details] [diff] [review]
(Av1) browser_library_infoBox.js: Add a missing |var childNode;|
[Checked in: See comment 18]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10)

> This is a very odd place to define childNode. Why not just change the
> existing line (below what's visible here)?

Yeah, but I preferred to place it where the history code (thus this |var|) is in Firefox test.

> And what is the comment supposed to mean? Is it actually necessary? What is
> the context (code wise; I guess it's generally referring to the Bookmarks
> Manager)?

I agree my comment is not ideal: I meant to document the code difference between FF and SM...
Maybe more like "Note: SeaMonkey handles bookmarks only, unlike Firefox which handles history too."?
Attachment #595612 - Flags: review?(jh)
(In reply to Serge Gautherie (:sgautherie) from comment #7)
> http://hg.mozilla.org/comm-central/rev/185700094ffb
> Bv1, with comment 6 suggestion(s).

(In reply to Serge Gautherie (:sgautherie) from comment #11)
> http://hg.mozilla.org/comm-central/rev/4323497b9d80
> Cv1, with comment 8 suggestion(s).

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328933833.1328938285.31695.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/10 20:17:13

V.Fixed, these 2.
(In reply to Serge Gautherie (:sgautherie) from comment #12)
> > This is a very odd place to define childNode. Why not just change the
> > existing line (below what's visible here)?
> 
> Yeah, but I preferred to place it where the history code (thus this |var|)
> is in Firefox test.

Well then personally I prefer to just have the "var" (or "let" if it wasn't for consistency) addition and nothing else. If someone is really interested in knowing the differences to the FF test code, they can always RTFS, no? I see little benefit otherwise, and this is a SM test, so we don't need do document each and every deviation from FF. I guess you know the details now, and anyone else will be able to read them here when coming from Blame. ;-)

So, either r=me for just the "var" addition, or you'll have to ask someone else.
Comment on attachment 595612 [details] [diff] [review]
(Av1) browser_library_infoBox.js: Add a missing |var childNode;|
[Checked in: See comment 18]

I'm marking r+ since I'm ok with you landing this with my (and Jens's) nit.

Please stick the var to define this where it is used (~line 84 in this file) rather than up here where its not.
Attachment #595612 - Flags: review?(jh)
Attachment #595612 - Flags: review?(bugspam.Callek)
Attachment #595612 - Flags: review+
Comment on attachment 595774 [details] [diff] [review]
(Dv1) Remove 3 superfluous jsm imports
[Checked in: Comment 21]

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

I'm curious, does the additional import here hurt anything, note re-importing any jsm still uses the same JSContext for the import and doesn't re-read/re-parse any JS
(In reply to Justin Wood (:Callek) from comment #16)
> I'm curious, does the additional import here hurt anything

I assume it doesn't: just redundant/useless.
Comment on attachment 595612 [details] [diff] [review]
(Av1) browser_library_infoBox.js: Add a missing |var childNode;|
[Checked in: See comment 18]

http://hg.mozilla.org/comm-central/rev/efc0e0fae87c
Av1, with comment 14 and comment 15 suggestion(s).
Attachment #595612 - Attachment description: browser_library_infoBox.js: Add a missing |var childNode;| → (Av1) browser_library_infoBox.js: Add a missing |var childNode;| [Checked in: See comment 18]
(In reply to Serge Gautherie (:sgautherie) from comment #18)
> http://hg.mozilla.org/comm-central/rev/efc0e0fae87c
> Av1, with comment 14 and comment 15 suggestion(s).

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329103195.1329108207.32214.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/12 19:19:55

V.Fixed, this 1 too.
Comment on attachment 595774 [details] [diff] [review]
(Dv1) Remove 3 superfluous jsm imports
[Checked in: Comment 21]

I take it we don't need these because those are browser-chrome tests and therefore utilityOverlay.js already imports them for us?
Attachment #595774 - Flags: review?(neil) → review+
Comment on attachment 595774 [details] [diff] [review]
(Dv1) Remove 3 superfluous jsm imports
[Checked in: Comment 21]

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


(In reply to neil@parkwaycc.co.uk from comment #20)

> I take it we don't need these because those are browser-chrome tests and

Yes, I assume browser-chrome is the key to these cases.

> therefore utilityOverlay.js already imports them for us?

I don't know which imports apply, but it could likely be those in utilityOverlay.js.
Iiuc Services is supposed to be available globally and I assume the same is true for XPCOMUtils.

http://mxr.mozilla.org/comm-central/search?string=Services.jsm&case=on&find=%2Fsuite%2F
http://mxr.mozilla.org/comm-central/search?string=XPCOMUtils.jsm&case=1&find=%2Fsuite%2F
And fwiw
http://mxr.mozilla.org/mozilla-central/search?string=XPCOMUtils.jsm&case=1&find=%2Fbrowser%2F.*%2Fbrowser_
"No results found"
http://mxr.mozilla.org/mozilla-central/search?string=Services.jsm&case=1&find=%2Fbrowser%2F.*%2Fbrowser_
"Found one matching line" : I filed bug 727682.
Attachment #595774 - Attachment description: (Dv1) Remove 3 superfluous jsm imports → (Dv1) Remove 3 superfluous jsm imports [Checked in: Comment 21]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.