Initialize the hidden private window lazily

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla21
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ fixed, firefox21 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch Patch (v1) (obsolete) — Splinter Review
This makes the code a bit more complicated but it's better than regressing Ts.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #698500 - Flags: review?(bzbarsky)
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"?
Component: Private Browsing → General
Product: Firefox → Core
(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.
Posted patch Patch (v2) (obsolete) — Splinter Review
The previous patch had a stupid bug which created on hidden private window every time you attempted to call the getter.
Attachment #698500 - Attachment is obsolete: true
Attachment #698500 - Flags: review?(bzbarsky)
Attachment #698716 - Flags: review?(bzbarsky)
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...
Attachment #698716 - Flags: review?(bzbarsky) → review+
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
Attachment #698716 - Attachment is obsolete: true
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.
Attachment #699248 - Flags: approval-mozilla-aurora?
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.
No longer blocks: 827460
Duplicate of this bug: 827460
This did fix the Ts regressions that we had seen in bug 815847 on inbound.
https://hg.mozilla.org/mozilla-central/rev/a65487a1e6de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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 .
Attachment #699248 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.