quora.com bloats out of control (part 3)

RESOLVED FIXED

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({mlk, regression})

Trunk
x86
Linux
mlk, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 511960 [details] [diff] [review]
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.
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.
(Assignee)

Comment 3

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

Updated

7 years ago
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.
Created attachment 512148 [details] [diff] [review]
Like so, say

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

Comment 7

7 years ago
Created attachment 512229 [details] [diff] [review]
v2
Attachment #511960 - Attachment is obsolete: true
Attachment #512148 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
Created attachment 512231 [details] [diff] [review]
v2
Attachment #512229 - Attachment is obsolete: true
Comment on attachment 512231 [details] [diff] [review]
v2

r=jst
Attachment #512231 - Flags: review+
Landed:

http://hg.mozilla.org/mozilla-central/rev/0fd8793db7f3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 11

7 years ago
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 → ---
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
Created attachment 512772 [details] [diff] [review]
Part 2 v1

This fixes the test failures.
Attachment #512772 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

7 years ago
Boris: you were worried about reloads, there are mochitests for reloads and all mochitests pass with this patch.
(Assignee)

Comment 15

7 years ago
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+
(Assignee)

Updated

7 years ago
Attachment #512772 - Flags: review+
(Assignee)

Comment 16

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

Comment 18

7 years ago
(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.
(Assignee)

Comment 19

7 years ago
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.
(Assignee)

Comment 20

7 years ago
Created attachment 513092 [details] [diff] [review]
Part 3 v1

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

Comment 21

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

Restoring r=bz.
Attachment #512772 - Flags: review+
(Assignee)

Comment 22

7 years ago
Created attachment 513094 [details] [diff] [review]
Rollup patch of the 3 parts, not for checkin

Fixes non-shutdown leaks caused by sites using either designMode or contentEditable.
Attachment #513094 - Flags: review+
Attachment #513094 - Flags: approval2.0?

Updated

7 years ago
Attachment #513094 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/9bac5cfcf7f2
http://hg.mozilla.org/mozilla-central/rev/5b5246f1e287
http://hg.mozilla.org/mozilla-central/rev/4b11eb3c12a0
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 635532
Keywords: mlk
Blocks: 659855
No longer blocks: 632234
You need to log in before you can comment on or make changes to this bug.