866223 - Compose windows are no longer recycled

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[[e-mail]]

Bug 777063 has a logic error in its code to obtain an nsIXULWindow from an nsIDOMWindow so it fails to recycle any compose windows.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Transferred from bug 814651 as requested.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 742490 [details] [diff] [review]
Proposed patch

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

Thanks for new crisp bug for this regression.

(In reply to [e-mail] from comment #0)
> so it fails to recycle any compose windows.

Was the failure "don't recycle" only?
  Is the failure simply "worked as if mail.compose.max_recycled_windows=0 always" only?
  Is possible resource leak issue involved in the "fails to recycle"?
  (resource leak issue of bug 818607 even after fix of Bug 765074)

Comment 3

[[e-mail]]

(In reply to WADA from comment #2)
> (In reply to comment #0)
> > so it fails to recycle any compose windows.
> 
> Was the failure "don't recycle" only?
>   Is the failure simply "worked as if mail.compose.max_recycled_windows=0
>   always" only?
That appears to be the case, yes.

>   Is possible resource leak issue involved in the "fails to recycle"?
>   (resource leak issue of bug 818607 even after fix of Bug 765074)
Fixing this bug may conceal the leak because of the recycling effect.
(Presumably setting max_recycled_windows=0 would then expose the leak again.)

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 742490 [details] [diff] [review]
Proposed patch

Yep, looks good. Tested the compose window is now cached.

Comment 5

[[e-mail]]

Pushed comm-central changeset c448a8a204be.

Comment 6

[WADA:World Anti-bad-Duping Agency]

Ccked with following trunk nightly build.
> Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20100101 Thunderbird/23.0a1
> Application Build ID 	: 20130504031036

(0) mail.compose.max_recycled_windows=5
(1) Open compose window, get DOM and XUL window via Window Enumerator.
    Call window objects "DOM-win-X" and "XUL-win-X".
    DOM-win-X.isCachedWin = false
(2) Keep reference to the DOM-win-X, XUL-win-X
(3) Close composition window, with keeping reference to window objects.
    i.e. "destory XUL window" event notification is ignored.
    => Window object is not returned by DOM and XUL Window Enumerator
    => DOM-win-X.isCachedWin = true
(4) Open composition window again
    => Same window object is returned by DOM and XUL Window Enumerator
    => DOM-win-X.isCachedWin = false
Composition window is cached and is re-used as designed => VERIFIED

Comment 7

[WADA:World Anti-bad-Duping Agency]

This bug causes severe memory leak problem of bug 818607(memory leak by Bug 765074), which was exposed to Tb 17 users due to this bug, even though many Tb users use mail.compose.max_recycled_windows>0(default is 1 or 5).
I believe patch of this bug is better landed on Tb 17 ESR.

Comment 8

[[e-mail]]

Comment on attachment 742490 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): 777063
User impact if declined: Performance impacted
  (compose windows slow to open and close and leak memory)
Testing completed (on c-c, etc.): Verified on c-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 742490 [details] [diff] [review]
Proposed patch

a=me for Aurora.

For esr17, we'll wait until the next cycle, so this has some testing (and we're going to cut it in the next 24 hours ish).

It is up to SeaMonkey if they want to land this on beta for the next release (no more TB betas for this cycle), but there's an implicit a=me if they want it.

Comment 10

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/7c915bfda901

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/cec6ac1b39f6