Last Comment Bug 866223 - Compose windows are no longer recycled
: Compose windows are no longer recycled
Status: VERIFIED FIXED
[regression:tb17][regression started ...
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 17 Branch
: x86 All
: -- major (vote)
: Thunderbird 23.0
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on:
Blocks: 814651 777063 818607
  Show dependency treegraph
 
Reported: 2013-04-26 11:47 PDT by neil@parkwaycc.co.uk
Modified: 2013-06-19 02:44 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
+
fixed
22+
fixed


Attachments
Proposed patch (944 bytes, patch)
2013-04-26 11:57 PDT, neil@parkwaycc.co.uk
standard8: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-04-26 11:47:32 PDT
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 neil@parkwaycc.co.uk 2013-04-26 11:57:41 PDT
Created attachment 742490 [details] [diff] [review]
Proposed patch

Transferred from bug 814651 as requested.
Comment 2 WADA 2013-04-28 02:38:17 PDT
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)
Comment 3 neil@parkwaycc.co.uk 2013-04-28 02:46:55 PDT
(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, limited time in Dec) 2013-05-01 04:06:31 PDT
Comment on attachment 742490 [details] [diff] [review]
Proposed patch

Yep, looks good. Tested the compose window is now cached.
Comment 5 neil@parkwaycc.co.uk 2013-05-01 16:03:42 PDT
Pushed comm-central changeset c448a8a204be.
Comment 6 WADA 2013-05-05 00:25:43 PDT
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 2013-05-05 01:05:46 PDT
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 neil@parkwaycc.co.uk 2013-05-05 02:03:25 PDT
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, limited time in Dec) 2013-05-09 03:09:27 PDT
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, limited time in Dec) 2013-05-09 03:36:33 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/7c915bfda901
Comment 11 Mark Banner (:standard8, limited time in Dec) 2013-06-19 02:44:50 PDT
https://hg.mozilla.org/releases/comm-esr17/rev/cec6ac1b39f6

Note You need to log in before you can comment on or make changes to this bug.