Closed Bug 866223 Opened 7 years ago Closed 7 years ago

Compose windows are no longer recycled

Categories

(Thunderbird :: Message Compose Window, defect, major)

17 Branch
x86
All
defect
Not set
major

Tracking

(thunderbird20 wontfix, thunderbird21 wontfix, thunderbird22+ fixed, thunderbird-esr1722+ fixed)

VERIFIED FIXED
Thunderbird 23.0
Tracking Status
thunderbird20 --- wontfix
thunderbird21 --- wontfix
thunderbird22 + fixed
thunderbird-esr17 22+ fixed

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [regression:tb17][regression started from Tb 17 by 777063] )

Attachments

(1 file)

Bug 777063 has a logic error in its code to obtain an nsIXULWindow from an nsIDOMWindow so it fails to recycle any compose windows.
Attached patch Proposed patchSplinter Review
Transferred from bug 814651 as requested.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #742490 - Flags: review?(mbanner)
Attachment #742490 - Flags: feedback?(m-wada)
Whiteboard: [regression started from Tb 17 by 777063]
Version: Trunk → 17
Whiteboard: [regression started from Tb 17 by 777063] → [regression:tb17][regression started from Tb 17 by 777063]
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 neil@parkwaycc.co.uk 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)
Attachment #742490 - Flags: feedback?(m-wada)
(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 on attachment 742490 [details] [diff] [review]
Proposed patch

Yep, looks good. Tested the compose window is now cached.
Attachment #742490 - Flags: review?(mbanner) → review+
Pushed comm-central changeset c448a8a204be.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
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
Status: RESOLVED → VERIFIED
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 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
Attachment #742490 - Flags: approval-comm-esr17?
Attachment #742490 - Flags: approval-comm-beta?
Attachment #742490 - Flags: approval-comm-aurora?
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.
Attachment #742490 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #742490 - Flags: approval-comm-beta?
Attachment #742490 - Flags: approval-comm-esr17? → approval-comm-esr17+
You need to log in before you can comment on or make changes to this bug.