725529 - Fix 4 "leaked window property" in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Testing Infrastructure, defect, P3)

Description

[Serge Gautherie (:sgautherie)]

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
}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) browser_library_infoBox.js: Add a missing |var childNode;| [Checked in: See comment 18]

Fix nit from bug 557496.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) browser_sanitizer.js: Load Sanitizer.jsm through a tempScope [Checked in: See comment 7]

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) browser_346337.js: Add 2 missing |var| [Checked in: See comment 11]

Port
http://hg.mozilla.org/mozilla-central/rev/f92194f05828
"No bug - fix some browser chrome tests that pollute the global scope"

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Remove 3 superfluous jsm imports [Checked in: Comment 21]

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...

Comment 5

[Serge Gautherie (:sgautherie)]

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...

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

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.

Comment 7

[Serge Gautherie (:sgautherie)]

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).

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

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.]

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(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 10

[Jens Hatlak (:InvisibleSmiley)]

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 11

[Serge Gautherie (:sgautherie)]

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 ;-)

Comment 12

[Serge Gautherie (:sgautherie)]

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."?

Comment 13

[Serge Gautherie (:sgautherie)]

(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.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(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 15

[Justin Wood (:Callek)]

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.

Comment 16

[Justin Wood (:Callek)]

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

Comment 17

[Serge Gautherie (:sgautherie)]

(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 18

[Serge Gautherie (:sgautherie)]

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).

Comment 19

[Serge Gautherie (:sgautherie)]

(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 20

[[e-mail]]

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?

Comment 21

[Serge Gautherie (:sgautherie)]

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 [e-mail] 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.