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

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 1

[(no longer active)]

In fact, swapping the docshells may just work out of the box.  Are we only worried about cookies or other things too?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

(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

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 4

[(no longer active)]

(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.

Comment 5

[(no longer active)]

(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.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

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

Comment 7

[(no longer active)]

(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.  :/

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 9

[(no longer active)]

Let's first decide what we want to do with the hidden window stuff...

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 11

[(no longer active)]

(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.

Comment 12

[(no longer active)]

Taking.  I'll try to prepare a patch here tonight.

Comment 13

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 14

[(no longer active)]

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 15

[Boris Zbarsky [:bzbarsky]]

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).

Comment 16

[(no longer active)]

Attachment: Patch (v2)

Actually that patch was not quite enough.  (I'll do those checks that you requested.)

Comment 17

[(no longer active)]

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 18

[Boris Zbarsky [:bzbarsky]]

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

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

r=me

Comment 19

[(no longer active)]

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.)

Comment 20

[Boris Zbarsky [:bzbarsky]]

I don't understand our startup cache stuff well enough to answer that...  Benjamin, do you?

Comment 21

[(no longer active)]

Also CCing mwu...  Please see comment 19.

Comment 22

[Benjamin Smedberg]

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.

Comment 23

[(no longer active)]

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?

Comment 24

[Josh Matthews [:jdm]]

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.

Comment 25

[Josh Matthews [:jdm]]

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.

Comment 26

[(no longer active)]

(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...

Comment 27

[(no longer active)]

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?

Comment 28

[(no longer active)]

(And BTW, you're right about last-pb-context-exiting being fine.)

Comment 29

[(no longer active)]

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!

Comment 30

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 31

[(no longer active)]

(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 32

[(no longer active)]

Attachment: Part 2: Ignore the hidden private window when sending the last-pb-context-exited notification

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691205 [details] [diff] [review]
Part 2: Ignore the hidden private window when sending the last-pb-context-exited notification

r=me

Comment 34

[Boris Zbarsky [:bzbarsky]]

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.

Comment 35

[(no longer active)]

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...

Comment 36

[(no longer active)]

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?

Comment 37

[(no longer active)]

Attachment: The printf debugging patch that I used

Intended for future reference for myself, please ignore this!

Comment 38

[Josh Matthews [:jdm]]

Ehsan, is this something that needs to be focused on soon/immediately?

Comment 39

[(no longer active)]

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.

Comment 40

[(no longer active)]

Benjamin, Michael, please see comment 36.

Comment 41

[Benjamin Smedberg]

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.

Comment 42

[(no longer active)]

(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!

Comment 43

[(no longer active)]

Attachment: Part 3: Remove nsXULPrototypeCache::gStartupCache

Comment 44

[(no longer active)]

Attachment: Part 4: Avoid double-caching

Comment 45

[(no longer active)]

Attachment: Part 4: Avoid double-caching

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.)

Comment 46

[(no longer active)]

Attachment: Waste less memory in nsXULPrototypeCache::mCacheURITable

Comment 47

[(no longer active)]

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

Comment 48

[(no longer active)]

Shoot, that was an empty try push!  This one isn't: https://tbpl.mozilla.org/?tree=Try&rev=305dfbcc8bfd

Comment 49

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1871463a0542
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d59903b166f
https://hg.mozilla.org/integration/mozilla-inbound/rev/434aeb3f910f
https://hg.mozilla.org/integration/mozilla-inbound/rev/63383825274e
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c6a3cf8a77

Comment 50

[Ed Morley [:emorley]]

Was this expected to cause a ton of Trace Malloc and Ts Paint regressions?

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1871463a0542
https://hg.mozilla.org/mozilla-central/rev/1d59903b166f
https://hg.mozilla.org/mozilla-central/rev/434aeb3f910f
https://hg.mozilla.org/mozilla-central/rev/63383825274e
https://hg.mozilla.org/mozilla-central/rev/16c6a3cf8a77

Comment 52

[(no longer active)]

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

Comment 53

[Ed Morley [:emorley]]

(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

Comment 54

[(no longer active)]

Hmm, Boris, do you see any way we can avoid these regressions?  I'm not sure I do :(

Comment 55

[Boris Zbarsky [:bzbarsky]]

I don't know.  :(

Comment 56

[(no longer active)]

Hrm, we can't create the hidden private window lazily... or can we?

Comment 57

[Erik Vold [:erikvold] (please needinfo? me)]

Where are the tests for `hiddenPrivateDOMWindow` ? all I see is a tests that it exists and one that it is private, nothing using it.

Comment 58

[(no longer active)]

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

Comment 59

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 60

[(no longer active)]

(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. :(