Document that has been navigated away does not get released if its container is hidden

RESOLVED FIXED in mozilla19



7 years ago
6 years ago


(Reporter: Felipe, Assigned: Felipe)


(Blocks 1 bug)

Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [MemShrink:P3])


(1 attachment, 1 obsolete attachment)



7 years ago
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?
bfcache, perhaps?
This sounds just like bug 728426 - we probably want to investigate a patch similar to that one for our sidebar.
Or ideally - a Core fix that makes such workarounds unnecessary!

Comment 4

7 years ago
FWIW, I have disablehistory="true" in that browser element, which hopefully should disable bfcache?

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

7 years ago
fwiw simply adding
> sbrowser.docShell.createAboutBlankContentViewer(null);

didn't help in this case
Is there some reason you don't want to remove the <browser> from the DOM?

Comment 7

7 years ago
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:

Comment 8

7 years ago
(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

7 years ago
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
ocumentViewer.cpp, line 1666

###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file c
/src/nsDocument.cpp, line 1578

are they problematic?
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

7 years ago
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


Comment 12

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


7 years ago
Assignee: nobody → felipc
Whiteboard: [MemShrink]

Comment 14

7 years ago
Posted patch Patch (obsolete) — Splinter Review
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 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.
Attachment #673663 - Flags: review?(bzbarsky)


7 years ago
Attachment #673663 - Attachment is patch: true
> 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 = 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

7 years ago
Comment on attachment 673663 [details] [diff] [review]

> 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
Attachment #673663 - Flags: review?(bzbarsky)
Whiteboard: [MemShrink] → [MemShrink:P3]

Comment 17

7 years ago
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.
Assignee: felipc → nobody

Comment 18

7 years ago
Posted patch PatchSplinter Review
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 (, now off to a full run at
Assignee: nobody → felipc
Attachment #673663 - Attachment is obsolete: true
Attachment #679959 - Flags: review?


7 years ago
Attachment #679959 - Flags: review? → review?(bzbarsky)
Comment on attachment 679959 [details] [diff] [review]

r=me, I think.  Fingers crossed!
Attachment #679959 - Flags: review?(bzbarsky) → review+


7 years ago
Blocks: 811089
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.