827188 - Initialize the hidden private window lazily

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

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

Comment 2

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=8c4e9d25b3a8

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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

Comment 4

[(no longer active)]

(In reply to comment #2)
> https://tbpl.mozilla.org/?tree=Try&rev=8c4e9d25b3a8
> 
> --- Comment #3 from :Gavin Sharp (use [e-mail] for email) <[e-mail]> 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.

Comment 5

[(no longer active)]

Attachment: Patch (v2)

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

Comment 6

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=0c5834c68961

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698716 [details] [diff] [review]
Patch (v2)

This looks fine at first glance...

Comment 11

[(no longer active)]

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

Comment 12

[(no longer active)]

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

Comment 13

[(no longer active)]

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

Comment 14

[(no longer active)]

Attachment: Patch for landing

Comment 15

[(no longer active)]

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 16

[(no longer active)]

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.

Comment 17

[(no longer active)]

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.

Comment 18

[(no longer active)]

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

Comment 20

[(no longer active)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a65487a1e6de

Comment 22

[bhavana bajaj [:bajaj]]

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 .

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7289797550a4