Initialize the hidden private window lazily

RESOLVED FIXED in Firefox 20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla21
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ fixed, firefox21 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 698500 [details] [diff] [review]
Patch (v1)

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
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 698716 [details] [diff] [review]
Patch (v2)

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)
(Assignee)

Comment 7

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

Comment 8

6 years ago
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+
(Assignee)

Comment 11

6 years ago
Pushed a debugging patch: https://tbpl.mozilla.org/?tree=Try&rev=cf0cf7dcc85b
(Assignee)

Comment 12

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

Comment 13

6 years ago
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
(Assignee)

Comment 14

6 years ago
Created attachment 699248 [details] [diff] [review]
Patch for landing
Attachment #698716 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
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?
(Assignee)

Updated

6 years ago
tracking-firefox20: --- → ?
(Assignee)

Comment 17

6 years ago
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.
Blocks: 827460
(Assignee)

Updated

6 years ago
No longer blocks: 827460
Duplicate of this bug: 827460
(Assignee)

Comment 20

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

6 years ago
status-firefox20: --- → affected
tracking-firefox20: ? → +
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7289797550a4
status-firefox20: affected → fixed
status-firefox21: --- → fixed
You need to log in before you can comment on or make changes to this bug.