Closed Bug 633738 Opened 9 years ago Closed 9 years ago

quora.com bloats out of control (part 3)

Categories

(Core :: DOM: Editor, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: peterv, Assigned: peterv)

References

Details

(Keywords: memory-leak, regression, Whiteboard: [softblocker])

Attachments

(4 files, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
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.
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.
I put a break point in DocumentViewerImpl::Destroy, mSHEntry is null so we never call SyncPresentationState from there.
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.
Depends on: 633879
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.
Attached patch Like so, say (obsolete) — Splinter Review
Peter, does this fix thing when used with the DropPresentationState change?
Attached patch v2 (obsolete) — Splinter Review
Attachment #511960 - Attachment is obsolete: true
Attachment #512148 - Attachment is obsolete: true
Attached patch v2Splinter Review
Attachment #512229 - Attachment is obsolete: true
Landed:

http://hg.mozilla.org/mozilla-central/rev/0fd8793db7f3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch Part 2 v1Splinter Review
This fixes the test failures.
Attachment #512772 - Flags: review?(bzbarsky)
Boris: you were worried about reloads, there are mochitests for reloads and all mochitests pass with this patch.
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.
Attachment #512772 - Flags: review?(bzbarsky) → review+
Attachment #512772 - Flags: review+
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?
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.
(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.
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.
Attached patch Part 3 v1Splinter Review
This just annotates the assertions from bug 439258 that we now trigger more.
Comment on attachment 512772 [details] [diff] [review]
Part 2 v1

Restoring r=bz.
Attachment #512772 - Flags: review+
Fixes non-shutdown leaks caused by sites using either designMode or contentEditable.
Attachment #513094 - Flags: review+
Attachment #513094 - Flags: approval2.0?
Attachment #513094 - Flags: approval2.0? → approval2.0+
Depends on: 635532
Keywords: mlk
No longer blocks: mlk2.0
You need to log in before you can comment on or make changes to this bug.