870101 - BackgroundPageThumbs should kill pages after capturing them

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

BackgroundPageThumbs destroys its browser after 60 seconds of inactivity, but it should evict pages from memory immediately after capturing them.  I tried to do that several ways in the original bug 841495 but couldn't get anything to work.

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Drew Willcoxon :adw from comment #0)
> BackgroundPageThumbs destroys its browser after 60 seconds of inactivity,
> but it should evict pages from memory immediately after capturing them.  I
> tried to do that several ways in the original bug 841495 but couldn't get
> anything to work.

Is the distinction here saving the larger screenshot of a page and retaining the small thumbnail, because the large screenshot can be read and may contain sensitive user data while the thumbnail likely does not?

Comment 2

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

This bug doesn't have anything to do with thumbnail sizes, it's just an implementation issue with the background service (it loads pages in the background, and doesn't unload them immediately after its taken the screenshot).

Comment 3

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: load about:blank after the capture

I'm sure I'm missing something obvious here - this patch simply loads about:blank after the capture is complete, which sounds like it should have the desired effect?  All tests in toolkit/components/thumbnails/test/ pass for me with this change.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 753124 [details] [diff] [review]
load about:blank after the capture

That's one of the things I tried, but the captured URL/window still appeared in about:memory, which led me to believe it didn't work.  That's what I think you need to do: try a technique and then reload about:memory to see if the window is still listed.  I could be misunderstanding how about:memory works, though; maybe it sometimes lists windows and related resources that are actually freed?  njn could probably help.

Comment 5

[Drew Willcoxon :adw]

One other thing about this patch: the captured page would still be in the bfcache.  It may be that loading about:blank is part of the solution, but doing that alone isn't enough I'm pretty sure.  I tried different ways of purging the bfcache but again couldn't get anything to work, judging from about:memory.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

You could add an 'unload' handler that would basically prevent the page from going into the bfcache. A little hacky maybe but I thought I'd mention it as an easy way out :)

Comment 7

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

You should be able to disable bfcache (and session history in general) by putting a disablehistory="true" on the <browser> (though I'm not sure offhand whether that works for remote browsers).

Comment 8

[Drew Willcoxon :adw]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> (though I'm not sure offhand whether that works for remote browsers).

It currently does not.

Comment 9

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I don't understand this yet.  What I have determined is:

* The fact this is a remote browser doesn't seem to change the behaviour - ie, doing everything in-process behaves the same.

* about:memory implies this isn't the BFCache - the window description in about:memory is prefixed by "cached/" when that is the case.  However, the cached/ prefix is actually determined by if window->IsFrozen() returns true.  It may just be that IsFrozen() is returning false even though the element is indeed the BFCache in this scenario.

* The fact we are loading a <browser> inside a HTML <iframe> seems a little suspect.  I'm wondering why we need to jump through these hoops, and can't use the remote browser element directly?  I don't understand why we need to be hosted under the hidden private window (mainly as IIRC, the hidden private window was added fairly late in the new-PB code, and mainly for jetpack) - isn't the 'browser.setAttribute("privatebrowsing", "true");' enough?  [Note this isn't rhetorical - I fully expect there *is* a reason I don't understand :]

Comment 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond (:markh) from comment #9)

> I don't understand why we need to
> be hosted under the hidden private window

It's because a docshell and all its children must all be in (or not in) PB mode (right?)

Comment 11

[Drew Willcoxon :adw]

(In reply to Mark Hammond (:markh) from comment #9)
> * The fact we are loading a <browser> inside a HTML <iframe> seems a little
> suspect.  I'm wondering why we need to jump through these hoops, and can't
> use the remote browser element directly?

See canHostBrowser in BackgroundPageThumbs.jsm.  If canHostBrowser returns false, then BackgroundPageThumbs._ensureParentWindowReady uses an html:iframe containing a document with the system principal.

> I don't understand why we need to be hosted under the hidden private window

It's a consequence of how we implemented private browsing in remote browsers.  Remote docshells inherit the PB state of their parent docshells in the chrome process, just as non-remote docshells inherit PB state from their parents.  We wanted the thumbnail browser to be private, so we used the hidden private window.

Originally I had a "privatebrowsing" attribute on the browser that was then forwarded to the remote docshell.  There wasn't any remote-inheriting of the PB state, so I used the regular hidden window.  But we settled on the implementation we have now.

Actually canHostBrowser isn't strictly necessary now that the private hidden window is used: canHostBrowser always returns false.  Back when the regular hidden window was used, canHostBrowser returned true on OS X and false everywhere else.  But I think it's nice and robust to leave it in.

Comment 12

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: demonstrate the leak

I think this is a platform leak, but sadly I still don't have much insight.  This attachment is a small repro of the leak (run it in a "browser" scratchpad) - it just creates a browser elt directly in the top-level document and demonstrates the exact same symptoms (ie, no hidden windows, no remote process, etc).  about:memory shows the phantom window and a debug-build shutdown shows the window being freed:

--DOMWINDOW == 11 (09834968) [serial = 34] [outer = 00000000] [url = http://www.mozilla.org/en-US/]
###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file o:/src/mm/mozilla-hg/mc-socialapi-landing/content/base/src/nsDocument.cpp, line 1422

I believe the assertion is due to us killing the document in its 'load' event, so probably isn't directly relevant.

I think we should probably open another bug for this, but thought I'd get some feedback from people here first...

Comment 13

[Drew Willcoxon :adw]

In this example you expect browser.docShell.createAboutBlankContentViewer(null) to free the window, is that right?  I don't know enough about that function to know if that's actually the case.  I think we should probably ask bz or someone who knows about window lifecycles.

bz, do you know how we can completely free a page and all its associated resources after loading it in a remote browser?

Comment 14

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

(In reply to Drew Willcoxon :adw from comment #11)
> Actually canHostBrowser isn't strictly necessary now that the private hidden
> window is used: canHostBrowser always returns false.  Back when the regular
> hidden window was used, canHostBrowser returned true on OS X and false
> everywhere else.  But I think it's nice and robust to leave it in.

(on a bit of a tangent here... ) I actually feel the opposite - it seems overkill to do a dynamic run-time check for something we know the answer to at build time (this was true even before, where we knew that Mac required one behavior and Linux/Windows another given the nature of their hidden windows). I think we should simplify this code as much as possible, similar to what bug 875496 did for the new tab preloader.

Comment 15

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

(In reply to Drew Willcoxon :adw from comment #13)
> In this example you expect
> browser.docShell.createAboutBlankContentViewer(null) to free the window, is
> that right?

That's just a workaround for the issue discovered in bug 728426, AIUI (though I'm not actually sure it applies in this scenario, since the browser in that snippet isn't hidden). The (inner) window should be freed when the document is navigated away from.

Is that particular test case getting tripped up by sessionhistory/bfcache? If you add disablehistory=true to the inserted <browser> does its behavior change? Is comment 8 the core bug here (we're getting session history when we don't want it, in the remote browser)?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Unloading a document does not immediately destroy it or its window.  Typically you need at least a CC + GC run for that.

Now of course maybe something is in fact holding on to the document or window...  Would have to do a heap dump to see what, I expect.

Comment 17

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I decided to try and find a "smaller" page to repro the problem on to help looking for what is keeping things alive and found something interesting - I found there were a number of sites it did not reproduce with.  Some trial-and-error later, I can't make it reproduce on a site that doesn't have <script> blocks - eg, it reproduces on www.google.com, but not on www.google.com/abc (a 404 - no <scripts> in it).  Also can't repro it on http://example.iana.org/ or http://www.gnu.org/.  Sadly though, I failed to reproduce it in a localhost server that *did* have <script> tags.  While this is far from conclusive, it seems interesting...

Unfortunately, I can't find a small enough site that it does repro it such that I can make sense of the GC info via the addons at https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump (which is the page bz pointed me at to try and help diagnose this).  I'm not sure where to go from here...

Comment 18

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

FTR, bug 709326 also has some discussions about windows/compartments hanging around much longer than expected and potentially requiring *many* CCs before they finally go away - some of that might be relevant.

Interestingly, the solution in that bug is simply to hide such windows which originated from a PB session from about:memory completely - which would mean the only symptom we have for the window remaining in memory would go away (ie, the window would still exist but not appear in about:memory).  I'm not sure if that is a good thing or not from the POV of this bug :)

Comment 19

[Drew Willcoxon :adw]

Attachment: patch

Mark, the verdict was that loading about:blank is actually sufficient because it eventually causes the captured window to be collected, right?  How's this patch?  I think there's a race condition in the one you posted: after the captured page loads and about:blank starts loading, _onCapture may be called again before about:blank finishes loading, so the currentURI.spec == "about:blank" check at the top of _onCapture may be false and not stop the about:blank load.  I think _onCapture should just unconditionally stop whatever's loading.

Comment 20

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 770374 [details] [diff] [review]
patch

Review of attachment 770374 [details] [diff] [review]:
-----------------------------------------------------------------

From memory, I think that webNav.currentURI will be about:blank even before it has finished loading - but yeah, that doesn't seem to be guarding anything worthwhile.

Comment 21

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd2f85714be

Comment 22

[Phil Ringnalda (:philor)]

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