Last Comment Bug 803255 - Document that has been navigated away does not get released if its container is hidden
: Document that has been navigated away does not get released if its container ...
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla19
Assigned To: :Felipe Gomes (needinfo me!)
:
Mentors:
Depends on:
Blocks: 466391 811089 802435
  Show dependency treegraph
 
Reported: 2012-10-18 13:25 PDT by :Felipe Gomes (needinfo me!)
Modified: 2013-01-18 11:17 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.29 KB, patch)
2012-10-20 23:26 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch (1.10 KB, patch)
2012-11-08 19:39 PST, :Felipe Gomes (needinfo me!)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-10-18 13:25:02 PDT
For the Social API, we have a sidebar (using a <browser> element) in the firefox window. The users can toggle the sidebar visibility on and off, which hides the sidebar without unloading it. (it sets hidden=true in the parent <vbox>)

After some inactivity, we actually unload that sidebar's content by navigating the <browser> element to about:blank. I can verify that about:blank loaded because scripts from the previous page stop running and accessing contentWindow shows it's in fact about:blank.

However, the compartment for the previous document hangs around forever (forcing CC and GC doesn't help). If then I unhide the sidebar, it immediatelly goes away. (it also goes away if I remove the <browser> from the DOM).

Andrew: I've got the cc-logs for this condition and I'll send it to you; if you could take a look to see what could be happening it would be much appreciated. Olli: any ideas on what could be holding on to these that goes away when the frame is unhidden? I don't think this is even the right component (maybe it's a display list bug?), but I'm filing it here for now. Also Shane said that he wasn't seeing this problem on Mac, so I wonder if this could be platform-specific?
Comment 1 Justin Lebar (not reading bugmail) 2012-10-18 13:27:56 PDT
bfcache, perhaps?
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-18 13:33:14 PDT
This sounds just like bug 728426 - we probably want to investigate a patch similar to that one for our sidebar.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-18 13:33:52 PDT
Or ideally - a Core fix that makes such workarounds unnecessary!
Comment 4 :Felipe Gomes (needinfo me!) 2012-10-18 13:46:08 PDT
FWIW, I have disablehistory="true" in that browser element, which hopefully should disable bfcache?  http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#709

Also it doesn't happen on every page (I can't reproduce with a simple webpage), but it happens with all the demo providers' pages that I've tested  (even when I disable all of the social code)

I will look at nsDocument::Release
Comment 5 :Felipe Gomes (needinfo me!) 2012-10-18 13:55:30 PDT
fwiw simply adding
> sbrowser.docShell.createAboutBlankContentViewer(null);

didn't help in this case
Comment 6 Justin Lebar (not reading bugmail) 2012-10-18 14:01:20 PDT
Is there some reason you don't want to remove the <browser> from the DOM?
Comment 7 :Felipe Gomes (needinfo me!) 2012-10-18 14:17:32 PDT
a breakpoint in nsDocument::Destroy (because it was easier) seems to indicate it's a similar problem. Stack:

> xul.dll!nsDocument::Destroy()  Line 7195	C++
> xul.dll!DocumentViewerImpl::Destroy()  Line 1617	C++
> xul.dll!DocumentViewerImpl::Show()  Line 1936	C++
> xul.dll!nsDocShell::SetVisibility(bool aVisibility=true)  Line 5281	C++
> xul.dll!nsFrameLoader::Show(int marginWidth=-1, int marginHeight=-1, int scrollbarPrefX=1, int scrollbarPrefY=1, nsSubDocumentFrame * frame=0x092e8640)  Line 831	C++
> xul.dll!nsSubDocumentFrame::ShowViewer()  Line 213 + 0x3d bytes	C++
> xul.dll!AsyncFrameInit::Run()  Line 104	C++
> xul.dll!nsContentUtils::RemoveScriptBlocker()  Line 5004	C++
> xul.dll!nsAutoScriptBlocker::~nsAutoScriptBlocker()  Line 2316	C++
> xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 3850	C++
> xul.dll!nsRefreshDriver::Notify(nsITimer * aTimer=0x0ae1e4b8)  Line 384	C++


which goes through: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1933
Comment 8 :Felipe Gomes (needinfo me!) 2012-10-18 14:24:17 PDT
(In reply to Justin Lebar [:jlebar] from comment #6)
> Is there some reason you don't want to remove the <browser> from the DOM?

that would require more invasive changes to the code that I'd really like to avoid if there is a simpler or more proper fix
Comment 9 :Felipe Gomes (needinfo me!) 2012-10-18 15:20:50 PDT
Setting the browser to about:blank and then calling `sbrowser.docShell.contentViewer.destroy()` works, but it prints these two assertions:

###!!! ASSERTION: Stop called too early or too late: 'mDocument', file
c:/moz/mozilla-central/ff-debug/layout/base/../../../mozilla/layout/base/nsD
ocumentViewer.cpp, line 1666

###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file c
:/moz/mozilla-central/ff-debug/content/base/src/../../../../mozilla/content/base
/src/nsDocument.cpp, line 1578

are they problematic?
Comment 10 Boris Zbarsky [:bz] 2012-10-18 17:38:47 PDT
So is the issue that paint suppression or some such is keeping the previous document alive?  Why is it actually not going away?
Comment 11 :Felipe Gomes (needinfo me!) 2012-10-18 23:33:52 PDT
Ok so here's more information. The stack I posted in comment 7 is from when I had already unloaded the content and then toggled the sidebar to be unhidden. Now, a more useful one shows how the clean-up happens when it's properly released (sidebar visible) and where it differs when the sidebar is hidden.  Here is the stack to DocumentViewerImpl::Destroy() when I set the sidebar to about:blank with the sidebar visible:

> xul.dll!DocumentViewerImpl::Destroy()  Line 1493	C++
> xul.dll!DocumentViewerImpl::Hide()  Line 2047	C++
> xul.dll!DocumentViewerImpl::InitInternal(nsIWidget * aParentWidget=0x00000000, nsISupports * aState=0x00000000, const nsIntRect & aBounds={...}, bool aDoCreation=true, bool aNeedMakeCX=true, bool aForceSetNewDocument=true)  Line 888	C++
> xul.dll!DocumentViewerImpl::Init(nsIWidget * aParentWidget=0x00000000, const nsIntRect & aBounds={...})  Line 678	C++
> xul.dll!nsDocShell::SetupNewViewer(nsIContentViewer * aNewViewer=0x14e807f8)  Line 8101 + 0x3b bytes	C++
> xul.dll!nsDocShell::Embed(nsIContentViewer * aContentViewer=0x14e807f8, const char * aCommand=0x564b9562, nsISupports * aExtraInfo=0x00000000)  Line 6153 + 0x1c bytes	C++
> xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x0ce26e60, nsIRequest * request=0x154b9f88, nsIStreamListener * * aContentHandler=0x0cc50800)  Line 7888 + 0x28 bytes	C++
> xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x0ce26e60, bool aIsContentPreferred=false, nsIRequest * request=0x154b9f88, nsIStreamListener * * aContentHandler=0x0cc50800, bool * aAbortProcess=0x0024d42b)  Line 122 + 0x20 bytes	C++
> xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x0a27b3b8, nsIChannel * aChannel=0x154b9f88)  Line 655 + 0x42 bytes	C++
> xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x14e0b918, nsISupports * aCtxt=0x00000000)  Line 356 + 0x35 bytes	C++
> xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x14e0b918, nsISupports * aCtxt=0x00000000)  Line 248 + 0x10 bytes	C++
> xul.dll!nsBaseChannel::OnStartRequest(nsIRequest * request=0x0ab63f40, nsISupports * ctxt=0x00000000)  Line 749 + 0x46 bytes	C++
> xul.dll!nsInputStreamPump::OnStateStart()  Line 417 + 0x2c bytes	C++
> xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x11b00410)  Line 368 + 0xb bytes	C++


Now, if I do the same but with the sidebar hidden, it gets to InitInternal, but Hide()* is not called, because mPresContext, aParentWidget and containerView are all null

* http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#885
Comment 12 :Felipe Gomes (needinfo me!) 2012-10-19 00:56:34 PDT
Oh, as I mentioned in comment 3, this doesn't happen for all pages. I just found a reduced testcase: only pages with event listeners leak. If the sidebar had the following url before being changed to about:blank:

> data:text/html,<body onclick='a()'>

it leaks, but it doesn't with

> data:text/html,<body>


Note that for the case where it doesn't leak, DocumentViewerImpl::Destroy() is still not called. So there might be two different problems at play here: for a visible <browser> the document is able to be force-cleared due to its contentViewer getting destroyed; and for an invisible one it's not cleared so there is a listener problem preventing the doc from being released
Comment 13 Boris Zbarsky [:bz] 2012-10-19 05:40:41 PDT
> Now, if I do the same but with the sidebar hidden, it gets to InitInternal, but Hide()*
> is not called, because mPresContext, aParentWidget and containerView are all null

OK.  That's broken.  That's what we should fix.  Even if we don't plan to call Hide(), we should drop mPreviousViewer as needed.
Comment 14 :Felipe Gomes (needinfo me!) 2012-10-20 23:26:43 PDT
Created attachment 673663 [details] [diff] [review]
Patch

bz: sorry, I spoke too soon about hide() working; I had made a change to the Hide function that was contributing to it. I had moved this block http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2048 to happen before the early return from the `if (!mPresShell)` check.  That seemed to me like a sane and simple change so I was going to propose we use that; however I sent it to try and it caused test failures, so it clearly isn't.

Here's an alternate approach that should have no implications outside of social: I just removed the [noscript] tag from the previousViewer attribute and then I clear it when unloading the page from the sidebar. No assertions are thrown with this and the document gets properly released. What do you think about that?

If that is not fine for beta I believe the only other option will be to remove the sidebar from the DOM.
Comment 15 Boris Zbarsky [:bz] 2012-10-21 19:10:15 PDT
> That seemed to me like a sane and simple change so I was going to propose we use that;
> however I sent it to try and it caused test failures

There are no sane and simple changes to document viewer code.  Hence my comments about not changing it on branches.  :(

> that should have no implications outside of social

Sure it does: it lets extensions mess with content viewer invariants...

In particular, setting .previousViewer to non-null, or worse yet to self, or setting it to null when the previous viewer has not had destroy() called on it (which, note, the attached patch does), is not great.

Exposing this sort of footgun to extensions does not make me very happy.  :(

I would almost rather we added, on branches, an entirely new interface that DocumentViewerImpl implements and that has a single method that executes this:

  if (mPreviousViewer) {
    mPreviousViewer->Destroy();
    mPreviousViewer = nullptr;
  }

and calling it from your code after QIing to this new interface.  Along with copious comments about how this interface will be going away and such (except of course beta is our ESR, so we'll be stuck with it there for a while, but I can live with that).

Does that sound plausible?

Another maybe plausible option is to change createAboutBlankContentViewer to nuke the previous viewer of the about:blank, since in that situation keeping it around is never desirable anyway.  Even that might be too risky for branches.  :(
Comment 16 :Felipe Gomes (needinfo me!) 2012-10-22 14:53:55 PDT
Comment on attachment 673663 [details] [diff] [review]
Patch

> Does that sound plausible?

Sure, yes. You raise a good point about beta being ESR so I'll first give it a stab at removing the element from the DOM. If it feels as too many changes for the UI code then I'll go with your proposed solution to create a function to destroy the previous viewer
Comment 17 :Felipe Gomes (needinfo me!) 2012-10-23 17:30:07 PDT
I wrote the patch to remove the element from the DOM and landed that together with the main patch in bug 802435. That should get us covered for now, and I'll try to get back to this bug soon-ish after the Social API work leaves me some free time.
Unassigning myself for now in case someone wants to take it.
Comment 18 :Felipe Gomes (needinfo me!) 2012-11-08 19:39:51 PST
Created attachment 679959 [details] [diff] [review]
Patch

So this implements the proper fix to the problem described in comment 11 and comment 13. I removed the workaround code from the frontend and verified that this fixes the leak.

Green run for OSX (https://tbpl.mozilla.org/?tree=Try&rev=f7a8f96e34b2), now off to a full run at https://tbpl.mozilla.org/?tree=Try&rev=5c07b119705a
Comment 19 Boris Zbarsky [:bz] 2012-11-10 00:03:57 PST
Comment on attachment 679959 [details] [diff] [review]
Patch

r=me, I think.  Fingers crossed!
Comment 20 :Felipe Gomes (needinfo me!) 2012-11-12 13:01:09 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5af1096232
Comment 21 Phil Ringnalda (:philor) 2012-11-12 21:19:19 PST
https://hg.mozilla.org/mozilla-central/rev/bd5af1096232

Note You need to log in before you can comment on or make changes to this bug.