Bug 815847 caused us to create one more window during startup, therefore regressing Ts. I have a patch to make that window lazily created, which makes the Ts regression go away.

This makes the code a bit more complicated but it's better than regressing Ts.

Comment on attachment 698500 [details] [diff] [review] Patch (v1) >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp > #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > #include "nsIPrivateBrowsingService.h" >+#else >+#include "nsIAppShellService.h" >+#include "nsAppShellCID.h" uber-nit: can you flip this around to an "ifdef" rather than an "ifndef" now that you're adding an "else"?

(In reply to comment #2) > https://tbpl.mozilla.org/?tree=Try&rev=8c4e9d25b3a8 > > --- Comment #3 from :Gavin Sharp (use gavin@gavinsharp.com for email) <gavin.sharp@gmail.com> 2013-01-06 21:21:06 EST --- > Comment on attachment 698500 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=698500 > Patch (v1) > > >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp > > > #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > > #include "nsIPrivateBrowsingService.h" > >+#else > >+#include "nsIAppShellService.h" > >+#include "nsAppShellCID.h" > > uber-nit: can you flip this around to an "ifdef" rather than an "ifndef" now > that you're adding an "else"? Sure, will do when landing.

The previous patch had a stupid bug which created on hidden private window every time you attempted to call the getter.

Re-pushed to try, this time with the correct trychoooser syntax: https://tbpl.mozilla.org/?tree=Try&rev=3139c075d303

Hmm, so this still leaks. I can't repro the leak locally when I run ./mach mochitest-chrome xpfe/appshell... Boris, can you think of why this would leak on Mac?

I dont know why it would leak, offhand. :(

Comment on attachment 698716 [details] [diff] [review] Patch (v2) This looks fine at first glance...

Pushed a debugging patch: https://tbpl.mozilla.org/?tree=Try&rev=cf0cf7dcc85b

Note to self: put the |NS_ENSURE_TRUE(mHiddenPrivateWindow, NS_ERROR_FAILURE);| in nsAppShellService::GetHiddenPrivateDOMWindow after the call to EnsurePrivateHiddenWindow before landing.

ehsan bz: so it turns out that if I use hiddenWindow.html instead of hiddenWindow.xul on Mac (only for the private hidden window), nothing leaks ehsan bz: I think that's acceptable... what do you think? bz ehsan: yes bz ehsan: using that for the private hidden window makes sense to me ehsan bz: ok, so can I land, r=you? bz ehsan: nice catch bz ehsan: yes ehsan thanks! :) ehsan bz: https://hg.mozilla.org/try/rev/8488b8c2c27b is the change I'm talking about, fwiw bz ehsan: yep, looks good

My experiments, for the record: https://tbpl.mozilla.org/?tree=Try&rev=8ee00f738fe9 https://tbpl.mozilla.org/?tree=Try&rev=9aa984265609 https://tbpl.mozilla.org/?tree=Try&rev=7bf3777a543c https://tbpl.mozilla.org/?tree=Try&rev=c4609c6c2b99 Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a6aff3dbbc

Comment on attachment 699248 [details] [diff] [review] Patch for landing Bug 815847 regressed Ts across the board, and this patch fixes that (without having any functional changes.) We should really take this patch on Aurora in order to undo those Ts regressions. The patch can be fairly risky but we have quite some time to fix any possible regressions. I'd rather take this sooner than later to make sure this does not mask other possible Ts changes.

Backed out because it breaks builds without MOZ_PER_WINDOW_PRIVATE_BROWSING :( https://hg.mozilla.org/integration/mozilla-inbound/rev/dfba6fd60061 Will push a fixed version in a sec.

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/a65487a1e6de

This did fix the Ts regressions that we had seen in bug 815847 on inbound.

Comment on attachment 699248 [details] [diff] [review] Patch for landing Approving on aurora. Please keep a close lookout on regressions considering we have already lost a week of aurora testing at this point .