633738 - quora.com bloats out of control (part 3)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

This part deals with the leak caused by keeping editor alive in the SHEntry, even though we don't keep anything else alive.

Boris suggested nsSHEntry::DropPresentationState should null out mEditorData. That seems like a good change, but doesn't affect this leak. The other suggestion was to not save the editor data if nsDocShell::DetachEditorFromWindow is called from nsDocShell::FirePageHideNotification and aIsUnload is false. The problem is that at that point we've already called nsDocShell::DetachEditorFromWindow from nsEditingSession::StartDocumentLoad. I'm not sure why we do that, seems like only calling nsDocShell::DetachEditorFromWindow from nsDocShell::FirePageHideNotification would be enough, but this code is all complicated and fragile :-/.

This patch removes the call from nsEditingSession::StartDocumentLoad and also hooks more of the editor to the CC. It seems to fix the remaining leaks.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The reason we want to avoid setting editor data when we're not persisting is because we won't save the document viewer, right?

If we make DropPresentationState null out mEditorData and then apply the patch in bug 633413, we should get that for free, because we'll call DropPresentationState if we're not persisting, I think.  Do you want to try that?  If that also fixes the remaining leaks, it would be safer than messing with nsEditingSession::StartDocumentLoad, I think.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, the SyncPresentationState that's already in the document viewer should be handling cases where we don't store the viewer in shistory, assuming DropPresentationState nulls out mEditorData.  So the editing session changes shouldn't be needed...  If it is (as in, if the editor CC stuff plus the DropPresentationState change doesn't fix the leak), I wonder what's going on.

Comment 3

[Peter Van der Beken [:peterv]]

I put a break point in DocumentViewerImpl::Destroy, mSHEntry is null so we never call SyncPresentationState from there.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah.  Yeah, ok.  We should SyncPresentationState on mOSHE (if it's not null) at the two places in nsDocShell.cpp where we do |mContentViewer->Close| conditional on mSavingOldViewer.  And we shouldn't condition that call on mContentViewer, so we drop things from the SHEntry even if !mContentViewer (though there should be nothing in the SHEntry in that case anyway).  That'll do the trick.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, I lied.  The call to SyncPresentationState should only happen if we didn't pass mOSHE to Close().  Otherwise we'll evict things when we shouldn't.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

Peter, does this fix thing when used with the DropPresentationState change?

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 512231 [details] [diff] [review]
v2

r=jst

Comment 10

[Johnny Stenback (:jst)]

Landed:

http://hg.mozilla.org/mozilla-central/rev/0fd8793db7f3

Comment 11

[(no longer active)]

I backed out the patch because of oranges:

mochitest-2: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297725042.1297725962.3719.gz
crashtest-debug: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297724786.1297725274.32614.gz

The backout changeset is: http://hg.mozilla.org/mozilla-central/rev/860a99144649

Comment 12

[Peter Van der Beken [:peterv]]

docshell/test/navigation/test_bug386782.html fails because we don't reattach the editor after navigating back. In RestoreFromHistory mSavingOldViewer is false. Trying to figure out why.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Part 2 v1

This fixes the test failures.

Comment 14

[Peter Van der Beken [:peterv]]

Boris: you were worried about reloads, there are mochitests for reloads and all mochitests pass with this patch.

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 512772 [details] [diff] [review]
Part 2 v1

Clearing review request for now, seems like there are still crashtest failures on try that I didn't see locally.

Comment 16

[Peter Van der Beken [:peterv]]

Boris, I'm a bit confused by the assertions, here's a stack:

###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()', file /builds/slave/try-osx64-dbg/build/layout/base/nsCSSFrameConstructor.cpp, line 11520
nsCSSFrameConstructor::RestyleForRemove [layout/base/nsCSSFrameConstructor.cpp:11522]
PresShell::ContentRemoved [layout/base/nsPresShell.cpp:5148]
nsHTMLEditor::DeleteRefToAnonymousNode [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:262]
nsHTMLEditor::HideInlineTableEditingUI [editor/libeditor/html/nsHTMLInlineTableEditor.cpp:150]
nsHTMLEditor::~nsHTMLEditor [editor/libeditor/html/nsHTMLEditor.cpp:178]
nsEditor::Release [editor/libeditor/base/nsEditor.cpp:217]
nsHTMLEditor::Release [editor/libeditor/html/nsHTMLEditor.cpp:281]
nsCOMPtr<nsIEditor>::assign_assuming_AddRef [nsCOMPtr.h:518]
nsCOMPtr<nsIEditor>::assign_with_AddRef [nsCOMPtr.h:1204]
nsCOMPtr<nsIEditor>::operator= [nsCOMPtr.h:664]
nsDocShellEditorData::TearDownEditor [docshell/base/nsDocShellEditorData.cpp:82]
nsDocShellEditorData::~nsDocShellEditorData [docshell/base/nsDocShellEditorData.cpp:73]
nsAutoPtr<nsDocShellEditorData>::assign [nsAutoPtr.h:71]
nsAutoPtr<nsDocShellEditorData>::operator= [nsAutoPtr.h:135]
nsSHEntry::DropPresentationState [docshell/shistory/src/nsSHEntry.cpp:766]
nsSHEntry::SetContentViewer [docshell/shistory/src/nsSHEntry.cpp:248]
nsSHistory::EvictContentViewersInRange [docshell/shistory/src/nsSHistory.cpp:932]
nsSHistory::EvictWindowContentViewers [docshell/shistory/src/nsSHistory.cpp:898]
nsSHistory::EvictContentViewers [docshell/shistory/src/nsSHistory.cpp:694]
DocumentViewerImpl::Show [layout/base/nsDocumentViewer.cpp:1976]
nsPresContext::EnsureVisible [layout/base/nsPresContext.cpp:1692]
PresShell::UnsuppressAndInvalidate [layout/base/nsPresShell.cpp:4614]
PresShell::ProcessReflowCommands [layout/base/nsPresShell.cpp:8047]
PresShell::FlushPendingNotifications [layout/base/nsPresShell.cpp:4901]
nsRefreshDriver::Notify [layout/base/nsRefreshDriver.cpp:327]

I can reproduce on Linux running the crashtests in editor/libeditor/html/crashtests. I don't really know what that assertion means, or why we'd trigger it when destroying the editor. Any ideas?

Comment 17

[Boris Zbarsky [:bzbarsky]]

That assert means editor is doing unsupported stuff, mostly.

Specifically, it calls a mutation observer without there being an actual DOM mutation going on...  And the frame constructor asserts that the call only happens for actual mutations.

We trigger it when destroying the editor, because that's when the editor decides to make its fake mutation-pretending call, as it tears down the anonymous content it created.

We should either hoist the check to presshell or silence the assetion for now.  Or flag the test as asserting.  Or change editor to pass a null aContainer, but that might confuse a11y... nothing else in ContentRemoved uses aContainer for the anonymous case.

Comment 18

[(no longer active)]

(In reply to comment #17)
> That assert means editor is doing unsupported stuff, mostly.
> 
> Specifically, it calls a mutation observer without there being an actual DOM
> mutation going on...  And the frame constructor asserts that the call only
> happens for actual mutations.
> 
> We trigger it when destroying the editor, because that's when the editor
> decides to make its fake mutation-pretending call, as it tears down the
> anonymous content it created.
> 
> We should either hoist the check to presshell or silence the assetion for now. 
> Or flag the test as asserting.  Or change editor to pass a null aContainer, but
> that might confuse a11y... nothing else in ContentRemoved uses aContainer for
> the anonymous case.

FWIW, I'm fine with annotating the assertion if you file a bug so that I fix this for real post 2.0.

Comment 19

[Peter Van der Beken [:peterv]]

Of course Jesse already filed a bug about those assertions (bug 439258). I'll post a rolled-up patch for approval once I get results from try.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Part 3 v1

This just annotates the assertions from bug 439258 that we now trigger more.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 512772 [details] [diff] [review]
Part 2 v1

Restoring r=bz.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Rollup patch of the 3 parts, not for checkin

Fixes non-shutdown leaks caused by sites using either designMode or contentEditable.

Comment 23

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/9bac5cfcf7f2
http://hg.mozilla.org/mozilla-central/rev/5b5246f1e287
http://hg.mozilla.org/mozilla-central/rev/4b11eb3c12a0