Need a way to open hidden DOM windows in private-browsing mode

RESOLVED FIXED in Firefox 20

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: evold, Assigned: Ehsan)

Tracking

unspecified
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

In the addon-sdk we swap windows from a hiddenDOMWindow to the active window for things like panels.  In this case we need to be able to open the hiddenDOMWindow in private browsing mode, or set it to private browsing mode to make sure that we don't send a window from a PB-enabled window to a PB-disabled hiddenDOMWindow.
In fact, swapping the docshells may just work out of the box.  Are we only worried about cookies or other things too?
(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> In fact, swapping the docshells may just work out of the box.  Are we only
> worried about cookies or other things too?

Just the stuff that a normal website visited from a tab would be subject to, https://support.mozilla.org/kb/private-browsing-browse-web-without-saving-info#w_what-does-private-browsing-not-save
No longer blocks: sdk-pwpb
(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> swapping the docshells may just work out of the box. 

So I think we move the docShell from one browser window to another, wherever the panel is opened, so you expect that the docShell would preserve state when moved to a hidden window and assume state when moved to a browser window?
(In reply to comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #1)
> > swapping the docshells may just work out of the box. 
> 
> So I think we move the docShell from one browser window to another, wherever
> the panel is opened, so you expect that the docShell would preserve state when
> moved to a hidden window and assume state when moved to a browser window?

Yes, it should.
(In reply to comment #2)
> (In reply to Ehsan Akhgari [:ehsan] from comment #1)
> > In fact, swapping the docshells may just work out of the box.  Are we only
> > worried about cookies or other things too?
> 
> Just the stuff that a normal website visited from a tab would be subject to,
> https://support.mozilla.org/kb/private-browsing-browse-web-without-saving-info#w_what-does-private-browsing-not-save

Well, but we're talking about XUL panels, so not everything there really applies.  For example, things such as history probably do not apply.
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Well, but we're talking about XUL panels, so not everything there really
> applies.  For example, things such as history probably do not apply.

A panel hosting a <browser> will have the problem though, right?
(In reply to comment #6)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > Well, but we're talking about XUL panels, so not everything there really
> > applies.  For example, things such as history probably do not apply.
> 
> A panel hosting a <browser> will have the problem though, right?

Hmm, I guess.  :/
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> (In reply to comment #2)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #1)
> > > In fact, swapping the docshells may just work out of the box.  Are we only
> > > worried about cookies or other things too?
> > 
> > Just the stuff that a normal website visited from a tab would be subject to,
> > https://support.mozilla.org/kb/private-browsing-browse-web-without-saving-info#w_what-does-private-browsing-not-save
> 
> Well, but we're talking about XUL panels, so not everything there really
> applies.  For example, things such as history probably do not apply.

The form/password/download history and cached content stuff seems useful.
Let's first decide what we want to do with the hidden window stuff...
Depends on: 816257
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Let's first decide what we want to do with the hidden window stuff...

Ehsan I agreed with you on the idea to use separate panels for pb windows in bug 816257 so we'll need to do this bug for pwpb.
(In reply to comment #10)
> (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> > Let's first decide what we want to do with the hidden window stuff...
> 
> Ehsan I agreed with you on the idea to use separate panels for pb windows in
> bug 816257 so we'll need to do this bug for pwpb.

Cool.
Taking.  I'll try to prepare a patch here tonight.
Assignee: nobody → ehsan
Posted patch Patch (v1) (obsolete) — Splinter Review
Please note that hiddenPrivateWindow and hiddenPrivateDOMWindow will not be available in non-per-window-PB builds.  Accessing them would just throw NS_ERROR_NOT_IMPLEMENTED.
Attachment #690707 - Flags: review?(bzbarsky)
Erik, you can use this patch in a local per-window PB build if you don't want to wait for this to land, but I'll try to land it as soon as possible.
Comment on attachment 690707 [details] [diff] [review]
Patch (v1)

This seems ok to me, but please check that closing all non-hidden windows still quits the app and such on Windows and Linux (that is, that we have no window-counting hackery that needs updating).
Attachment #690707 - Flags: review?(bzbarsky) → review+
Posted patch Patch (v2)Splinter Review
Actually that patch was not quite enough.  (I'll do those checks that you requested.)
Attachment #690707 - Attachment is obsolete: true
Attachment #690712 - Flags: review?(bzbarsky)
Blocks: 820254
Tested on Windows and things look fine.  Pushing to try since this might be a bit risky: https://tbpl.mozilla.org/?tree=Try&rev=f3effae64af8
Comment on attachment 690712 [details] [diff] [review]
Patch (v2)

Ah, here are the ConsiderQuitStopper bits I was expecting. ;)

r=me
Attachment #690712 - Flags: review?(bzbarsky) → review+
Hmm, I'm hitting an assertion with this early at startup:

 0:07.25 ###!!! ASSERTION: Existing entry in StartupCache.: 'entry == nullptr', file /Users/ehsanakhgari/moz/mozilla-central/startupcache/StartupCache.cpp, line 344
 0:07.27 mozilla::scache::StartupCache::PutBuffer(char const*, char const*, unsigned int)+0x000000FB [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0041BACB]
 0:07.27 nsXULPrototypeCache::FinishOutputStream(nsIURI*)+0x000002AB [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0104F42B]
 0:07.28 nsXULPrototypeCache::WritePrototype(nsXULPrototypeDocument*)+0x00000233 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0104EDE3]
 0:07.28 nsXULDocument::DoneWalking()+0x000003F3 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0103AFE3]
 0:07.28 nsXULDocument::ResumeWalk()+0x00001086 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0102F876]
 0:07.29 nsXULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*)+0x000007EB [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0103BF5B]
 0:07.29 non-virtual thunk to nsXULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*)+0x0000004D [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0103C03D]
 0:07.29 nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x000000D3 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x000F6F33]
 0:07.29 nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x00000104 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0007F774]
 0:07.30 non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x0000003D [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0007F89D]
 0:07.30 nsInputStreamPump::OnStateStop()+0x000001A3 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x000A02A3]
 0:07.30 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)+0x000000F7 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0009F7A7]
 0:07.30 non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)+0x0000002F [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x000A039F]
 0:07.31 nsInputStreamReadyEvent::Run()+0x00000090 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02674DF0]
 0:07.31 nsThread::ProcessNextEvent(bool, bool*)+0x00000676 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x026A66C6]
 0:07.31 NS_ProcessPendingEvents_P(nsIThread*, unsigned int)+0x000000AC [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-pb/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0260A4DC]

This seems to be because the hiddenWindow.xul entry now gets noted twice.  (I see two PutEntry calls for "xulcache/resource/gre/chrome/browser/content/browser/hiddenWindow.xul".  This assertion now seems clearly wrong in per-window PB builds.  Can I go ahead and disable it?  (I can disable it only for hiddenWindow.xul, if you want me to.)
I don't understand our startup cache stuff well enough to answer that...  Benjamin, do you?
Also CCing mwu...  Please see comment 19.
That assertion is correct: clients of the startup cache are not supposed to put entries which already exist: they are supposed to have read the existing entry.
Another thing that this patch breaks is last-pb-context-exited/exiting...  While I can hack around that and make the code which dispatches those notifications not take the hidden private window into account, that seems very risky for things like the DOM storage code.

I don't know how to solve that...  Josh, what do you think?
Does finding all navigator:browser windows return the hidden window? If not, then last-pb-context-exiting is fine. I don't have any ideas for the docshell tracking, though.
We could add a flag to the docshell or loadcontext interface indicating that its private docshells don't count, I guess, but the idea doesn't thrill me.
(In reply to comment #25)
> We could add a flag to the docshell or loadcontext interface indicating that
> its private docshells don't count, I guess, but the idea doesn't thrill me.

Well, sure, but the point is that if we don't track those docshells, we might break things which rely on pb-last-context-exited, such as DOM Storage.  If we do track those docshells, then we break other things which rely on pb-last-context-exited firing when the last visible window has been closed...
I guess one "solution" would be to add a new notification such as last-pb-docshell-destroyed, and make that fire when the last PB docshell dies (including the hidden one), and keep firing the last-pb-context-exited when the last PB docshell (excluding the hidden one) goes away.  That way we could hook up things which do cleanup to the former, and other things which do a user visible action (such as clearing the clipboard) to the latter.

What do you think, Josh?
(And BTW, you're right about last-pb-context-exiting being fine.)
OK, the downside of my solution in comment 27 is that some things will live past the lifetime of the last private window (example: dom storage entries.)  Josh suggested another solution, which is basically firing last-pb-context-exited at the same time that we do today (which means we will ignore hiddenPrivateWindow) and get the jetpack code to destroy the private iframe that it has around when last-pb-context-exited fires and recreate it the next time that we open a private window.  This will avoid the serious downside mentioned above, and will require Jetpack to adjust their code based on that.

Erik, is that acceptable to you?  Thanks!
(In reply to Ehsan Akhgari [:ehsan] from comment #29)
> OK, the downside of my solution in comment 27 is that some things will live
> past the lifetime of the last private window (example: dom storage entries.)
> Josh suggested another solution, which is basically firing
> last-pb-context-exited at the same time that we do today (which means we
> will ignore hiddenPrivateWindow) and get the jetpack code to destroy the
> private iframe that it has around when last-pb-context-exited fires and
> recreate it the next time that we open a private window.  This will avoid
> the serious downside mentioned above, and will require Jetpack to adjust
> their code based on that.
> 
> Erik, is that acceptable to you?  Thanks!

I don't see a problem with this.
(In reply to comment #30)
> (In reply to Ehsan Akhgari [:ehsan] from comment #29)
> > OK, the downside of my solution in comment 27 is that some things will live
> > past the lifetime of the last private window (example: dom storage entries.)
> > Josh suggested another solution, which is basically firing
> > last-pb-context-exited at the same time that we do today (which means we
> > will ignore hiddenPrivateWindow) and get the jetpack code to destroy the
> > private iframe that it has around when last-pb-context-exited fires and
> > recreate it the next time that we open a private window.  This will avoid
> > the serious downside mentioned above, and will require Jetpack to adjust
> > their code based on that.
> > 
> > Erik, is that acceptable to you?  Thanks!
> 
> I don't see a problem with this.

Great!  I'll prepare a new patch which does this tonight then.
Comment on attachment 691205 [details] [diff] [review]
Part 2: Ignore the hidden private window when sending the last-pb-context-exited notification

r=me
Attachment #691205 - Flags: review?(bzbarsky) → review+
So I just talked to Ehsan...  The risk from this stuff is pretty substantial.  He's going to propose something that might be safer and maybe work in the short run, but I think we should perhaps just work on a fix for bug 565388, especially since most of the pieces are already in place to do that sort of thing.
It seems like my plan (which was basically stick the iframe under the first opened private window, and try to put it in another private window when that one gets closed) won't quite work with the existing semantics of the API.  On Mac, where it's possible for us to have no windows open except for the special Mac hidden window, we may get into a situation where there is no appropriate window for the iframe to live under, which would cause us to have to destroy and recreate it if the user opens a new private window later on, which causes the document loaded in the panel to lose its state.

Boris is working on a possible way to fix bug 565388, and if that's possible, that would be preferable.  I'm also working to see if I can figure out the startup cache problem in the mean time...
So, here's what's going on with the startup cache.

When we first try to load hiddenWindow.xul, we call StartupCache::GetBuffer.  mStartupWriteInitiated is false at that time, and the hiddenWindow key does not exist in mTable, so we end up calling GetBufferFromZipArchive, but mArchive is null at that point, so that call fails <http://mxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.cpp#310>.  Here's what the call stack looks like:

#0  mozilla::scache::(anonymous namespace)::GetBufferFromZipArchive (zip=0x0, doCRC=true, id=0x112bb9168 "xulcache/resource/gre/chrome/browser/content/browser/hiddenWindow.xul", outbuf=0x7fff5fbfb038, length=0x7fff5fbfb034) at /Users/ehsanakhgari/moz/mozilla-central/startupcache/StartupCache.cpp:277
#1  0x00000001017f31c6 in mozilla::scache::StartupCache::GetBuffer (this=0x10050c030, id=0x112bb9168 "xulcache/resource/gre/chrome/browser/content/browser/hiddenWindow.xul", outbuf=0x7fff5fbfb038, length=0x7fff5fbfb034) at /Users/ehsanakhgari/moz/mozilla-central/startupcache/StartupCache.cpp:310
#2  0x0000000102425d8a in nsXULPrototypeCache::GetInputStream (this=0x100552d80, uri=0x10faf2e00, stream=0x7fff5fbfb130) at /Users/ehsanakhgari/moz/mozilla-central/content/xul/document/src/nsXULPrototypeCache.cpp:412
#3  0x0000000102424c37 in nsXULPrototypeCache::GetPrototype (this=0x100552d80, aURI=0x10faf2e00) at /Users/ehsanakhgari/moz/mozilla-central/content/xul/document/src/nsXULPrototypeCache.cpp:149
#4  0x0000000102404b7d in nsXULDocument::StartDocumentLoad (this=0x10056f000, aCommand=0x105078330 "view", aChannel=0x10eb646d0, aLoadGroup=0x10e995900, aContainer=0x10051d550, aDocListener=0x110f04bc0, aReset=true, aSink=0x0) at /Users/ehsanakhgari/moz/mozilla-central/content/xul/document/src/nsXULDocument.cpp:449
#5  0x00000001018e03bb in nsContentDLF::CreateXULDocument (this=0x11035af40, aCommand=0x105078330 "view", aChannel=0x10eb646d0, aLoadGroup=0x10e995900, aContentType=0x110349258 "application/vnd.mozilla.xul+xml", aContainer=0x10051d550, aExtraInfo=0x0, aDocListener=0x110f04bc0, aContentViewer=0x7fff5fbfbaf0) at /Users/ehsanakhgari/moz/mozilla-central/layout/build/nsContentDLF.cpp:451
#6  0x00000001018dfa0b in nsContentDLF::CreateInstance (this=0x11035af40, aCommand=0x105078330 "view", aChannel=0x10eb646d0, aLoadGroup=0x10e995900, aContentType=0x110349258 "application/vnd.mozilla.xul+xml", aContainer=0x10051d550, aExtraInfo=0x0, aDocListener=0x110f04bc0, aDocViewer=0x7fff5fbfbaf0) at /Users/ehsanakhgari/moz/mozilla-central/layout/build/nsContentDLF.cpp:242
#7  0x0000000102e92910 in nsDocShell::NewContentViewerObj (this=0x10051d400, aContentType=0x110349258 "application/vnd.mozilla.xul+xml", request=0x10eb646d0, aLoadGroup=0x10e995900, aContentHandler=0x110f04bc0, aViewer=0x7fff5fbfbaf0) at /Users/ehsanakhgari/moz/mozilla-central/docshell/base/nsDocShell.cpp:7947
#8  0x0000000102e90754 in nsDocShell::CreateContentViewer (this=0x10051d400, aContentType=0x110349258 "application/vnd.mozilla.xul+xml", request=0x10eb646d0, aContentHandler=0x110f04bc0) at /Users/ehsanakhgari/moz/mozilla-central/docshell/base/nsDocShell.cpp:7752
#9  0x0000000102eb6c54 in nsDSURIContentListener::DoContent (this=0x10f852d80, aContentType=0x110349258 "application/vnd.mozilla.xul+xml", aIsContentPreferred=false, request=0x10eb646d0, aContentHandler=0x110f04bc0, aAbortProcess=0x7fff5fbfbd07) at /Users/ehsanakhgari/moz/mozilla-central/docshell/base/nsDSURIContentListener.cpp:122
#10 0x0000000102ec2d02 in nsDocumentOpenInfo::TryContentListener (this=0x110f04ba0, aListener=0x10f852d80, aChannel=0x10eb646d0) at /Users/ehsanakhgari/moz/mozilla-central/uriloader/base/nsURILoader.cpp:658
#11 0x0000000102ec17a8 in nsDocumentOpenInfo::DispatchContent (this=0x110f04ba0, request=0x10eb646d0, aCtxt=0x0) at /Users/ehsanakhgari/moz/mozilla-central/uriloader/base/nsURILoader.cpp:360
#12 0x0000000102ec1135 in nsDocumentOpenInfo::OnStartRequest (this=0x110f04ba0, request=0x10eb646d0, aCtxt=0x0) at /Users/ehsanakhgari/moz/mozilla-central/uriloader/base/nsURILoader.cpp:252
#13 0x0000000101456d6d in nsBaseChannel::OnStartRequest (this=0x10eb64680, request=0x110391b80, ctxt=0x0) at /Users/ehsanakhgari/moz/mozilla-central/netwerk/base/src/nsBaseChannel.cpp:720
#14 0x0000000101457047 in non-virtual thunk to nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) () at /Users/ehsanakhgari/moz/mozilla-central/netwerk/base/src/nsBaseChannel.cpp:680
#15 0x0000000101477463 in nsInputStreamPump::OnStateStart (this=0x110391b80) at /Users/ehsanakhgari/moz/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:417
#16 0x0000000101477165 in nsInputStreamPump::OnInputStreamReady (this=0x110391b80, stream=0x10f857af8) at /Users/ehsanakhgari/moz/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:368
#17 0x0000000101477d7f in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) () at /Users/ehsanakhgari/moz/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:559
#18 0x0000000103a4cb90 in nsInputStreamReadyEvent::Run (this=0x110f06600) at /Users/ehsanakhgari/moz/mozilla-central/xpcom/io/nsStreamUtils.cpp:82
#19 0x0000000103a7e466 in nsThread::ProcessNextEvent (this=0x10053ac40, mayWait=false, result=0x7fff5fbfc563) at /Users/ehsanakhgari/moz/mozilla-central/xpcom/threads/nsThread.cpp:627

Now, I asked myself why mArchive is null.  Turns out that when we run Firefox with a new profile (which is the case when we run tests on it), LoadArchive called by StartupCahce::Init returns NS_ERROR_NOT_FOUND, because mFile does not exist, because we have not created the file yet.  It is not until WriteToDisk gets called later on and initializes mArchive that we populate the startup cache, and then we'll hit the assertion in comment 19.  That tells me that there isn't a bug in the caller of the statup cache here.  The only conclusion that I can reach at is that the assertion in StartupCache.cpp needs to be adjusted somehow (perhaps by remembering that we hit this case in GetBuffer before mArchive was initialized, and ignore the assertion in that case???).

Benjamin, Michael, what do you guys think?
Intended for future reference for myself, please ignore this!
Ehsan, is this something that needs to be focused on soon/immediately?
Flags: needinfo?(ehsan)
I'm not sure, I've been waiting for feedback from the Add-on SDK folks over in bug 816257.  That said, if you have the cycles it would be great to look into what I was debugging in comment 36 so that we have a ready-to-land solution here.  I believe that is the only problem with my current patches.
Flags: needinfo?(ehsan)
Benjamin, Michael, please see comment 36.
Flags: needinfo?(mwu)
Flags: needinfo?(benjamin)
Are you saying that nsXULPrototypeCache::FinishOutputStream is *not* being called twice for hiddenWindow.xul?

Comment 36 looks like normal startup cache operation when there is not a startup cache yet. GetBuffer will correctly fail on first run.

Callers are expected to track "pending" loads which might call PutBuffer and coalesce them.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> Are you saying that nsXULPrototypeCache::FinishOutputStream is *not* being
> called twice for hiddenWindow.xul?

It is being called twice for hiddenWindow.xul and the assertion triggers the second time that this function is called.

> Comment 36 looks like normal startup cache operation when there is not a
> startup cache yet. GetBuffer will correctly fail on first run.

OK.

> Callers are expected to track "pending" loads which might call PutBuffer and
> coalesce them.

I see, so that explains why the assertion is triggered.  Can you please point me to an example of a caller which does this kind of tracking?

Thanks!
Attachment #691222 - Attachment is obsolete: true
Attachment #697660 - Flags: review?(benjamin)
Posted patch Part 4: Avoid double-caching (obsolete) — Splinter Review
Attachment #697661 - Flags: review?(benjamin)
Actually this fix was way more involved.  The old semantics of mCacheURITable is no longer used, so this patch entirely repurposes it for making sure that we don't double-cache.  This needs another patch which converts this variable to an nsTHashtable, which I'll attach shortly (pardon the bad patch #'s, I'll fix them when landing.)
Attachment #697661 - Attachment is obsolete: true
Attachment #697661 - Flags: review?(benjamin)
Attachment #697677 - Flags: review?(benjamin)
Attachment #697660 - Flags: review?(benjamin) → review+
Attachment #697677 - Flags: review?(benjamin) → review+
Attachment #697679 - Flags: review?(benjamin) → review+
Shoot, that was an empty try push!  This one isn't: https://tbpl.mozilla.org/?tree=Try&rev=305dfbcc8bfd
Blocks: 816257
No longer depends on: 816257
Was this expected to cause a ton of Trace Malloc and Ts Paint regressions?
(In reply to comment #50)
> Was this expected to cause a ton of Trace Malloc and Ts Paint regressions?

It probably regresses things a bit, yes, since we're creating one more window during startup. :(  How much were the regressions?
(In reply to :Ehsan Akhgari from comment #52)
> It probably regresses things a bit, yes, since we're creating one more
> window during startup. :(  How much were the regressions?

Between 2 and 9% across 16 dev.tree-management regression emails.
Not going to paste them here due to the quantity.
https://groups.google.com/forum/#!forum/mozilla.dev.tree-management
Hmm, Boris, do you see any way we can avoid these regressions?  I'm not sure I do :(
I don't know.  :(
Hrm, we can't create the hidden private window lazily... or can we?
Depends on: 827188
Depends on: 827460
Where are the tests for `hiddenPrivateDOMWindow` ? all I see is a tests that it exists and one that it is private, nothing using it.
(In reply to comment #57)
> Where are the tests for `hiddenPrivateDOMWindow` ? all I see is a tests that it
> exists and one that it is private, nothing using it.

What do you mean by using it?
(In reply to :Ehsan Akhgari from comment #58)
> (In reply to comment #57)
> > Where are the tests for `hiddenPrivateDOMWindow` ? all I see is a tests that it
> > exists and one that it is private, nothing using it.
> 
> What do you mean by using it?

I'd figure there be tests that check the DOM works correctly, stuff like adding frames, running scripts, using DOM elements to load images or any number of things.
(In reply to comment #59)
> (In reply to :Ehsan Akhgari from comment #58)
> > (In reply to comment #57)
> > > Where are the tests for `hiddenPrivateDOMWindow` ? all I see is a tests that it
> > > exists and one that it is private, nothing using it.
> > 
> > What do you mean by using it?
> 
> I'd figure there be tests that check the DOM works correctly, stuff like adding
> frames, running scripts, using DOM elements to load images or any number of
> things.

No, we don't usually test everything that is possible on a document like that.  We mostly assume that if some of the things work (such as the docshell being properly marked as private) then the rest would work as well, which turned out to not be the case here. :(
Duplicate of this bug: 733009
Depends on: 1166356
You need to log in before you can comment on or make changes to this bug.