Attachment 341821 Details for Bug 186215 – Fixes the leak

[patch] Fixes the leak

bug-186215-20180601202421.patch (text/plain), 8.09 KB, created by Ryosuke Niwa on 2018-06-01 20:24:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-06-01 20:24:22 PDT

Size: 8.09 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 232433)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,22 @@
>+2018-06-01  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Editor can hold references to Documents after you navigate away
>+        https://bugs.webkit.org/show_bug.cgi?id=186215
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Clear the various member variables that can hold onto a document in Editor::clear and FrameSelection::prepareForDestruction.
>+
>+        Test: editing/selection/navigation-clears-editor-state.html
>+
>+        * editing/Editor.cpp:
>+        (WebCore::Editor::clear):
>+        * editing/Editor.h:
>+        * editing/FrameSelection.cpp:
>+        (WebCore::FrameSelection::FrameSelection):
>+        (WebCore::FrameSelection::prepareForDestruction):
>+        * editing/FrameSelection.h:
>+
> 2018-06-01  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         ResourceLoader::cancel() shouldn't synchronously fire load event on document
>Index: Source/WebCore/editing/Editor.cpp
>===================================================================
>--- Source/WebCore/editing/Editor.cpp	(revision 232420)
>+++ Source/WebCore/editing/Editor.cpp	(working copy)
>@@ -1165,6 +1165,7 @@ Editor::~Editor() = default;
> 
> void Editor::clear()
> {
>+    m_lastEditCommand = nullptr;
>     if (m_compositionNode) {
>         m_compositionNode = nullptr;
>         if (EditorClient* client = this->client())
>@@ -1173,6 +1174,14 @@ void Editor::clear()
>     m_customCompositionUnderlines.clear();
>     m_shouldStyleWithCSS = false;
>     m_defaultParagraphSeparator = EditorParagraphSeparatorIsDiv;
>+    m_mark = VisibleSelection();
>+    m_oldSelectionForEditorUIUpdate = VisibleSelection();
>+    m_editorUIUpdateTimer.stop();
>+
>+#if ENABLE(TELEPHONE_NUMBER_DETECTION) && !PLATFORM(IOS)
>+    m_telephoneNumberDetectionUpdateTimer.stop();
>+    m_detectedTelephoneNumberRanges.clear();
>+#endif
> }
> 
> bool Editor::insertText(const String& text, Event* triggeringEvent, TextEventInputType inputType)
>Index: Source/WebCore/editing/Editor.h
>===================================================================
>--- Source/WebCore/editing/Editor.h	(revision 232420)
>+++ Source/WebCore/editing/Editor.h	(working copy)
>@@ -582,8 +582,6 @@ private:
>     const std::unique_ptr<PAL::KillRing> m_killRing;
>     const std::unique_ptr<SpellChecker> m_spellChecker;
>     const std::unique_ptr<AlternativeTextController> m_alternativeTextController;
>-    VisibleSelection m_mark;
>-    bool m_areMarkedTextMatchesHighlighted { false };
>     EditorParagraphSeparator m_defaultParagraphSeparator { EditorParagraphSeparatorIsDiv };
>     bool m_overwriteModeEnabled { false };
> 
>@@ -592,6 +590,9 @@ private:
>     HashSet<String> m_removedAttachmentIdentifiers;
> #endif
> 
>+    VisibleSelection m_mark;
>+    bool m_areMarkedTextMatchesHighlighted { false };
>+
>     VisibleSelection m_oldSelectionForEditorUIUpdate;
>     Timer m_editorUIUpdateTimer;
>     bool m_editorUIUpdateTimerShouldCheckSpellingAndGrammar { false };
>Index: Source/WebCore/editing/FrameSelection.cpp
>===================================================================
>--- Source/WebCore/editing/FrameSelection.cpp	(revision 232420)
>+++ Source/WebCore/editing/FrameSelection.cpp	(working copy)
>@@ -123,7 +123,9 @@ FrameSelection::FrameSelection(Frame* fr
>     : m_frame(frame)
>     , m_xPosForVerticalArrowNavigation(NoXPosForVerticalArrowNavigation())
>     , m_granularity(CharacterGranularity)
>+#if ENABLE(TEXT_CARET)
>     , m_caretBlinkTimer(*this, &FrameSelection::caretBlinkTimerFired)
>+#endif
>     , m_appearanceUpdateTimer(*this, &FrameSelection::appearanceUpdateTimerFired)
>     , m_caretInsidePositionFixed(false)
>     , m_absCaretBoundsDirty(true)
>@@ -1542,6 +1544,8 @@ void FrameSelection::prepareForDestructi
> 
>     setSelectionWithoutUpdatingAppearance(VisibleSelection(), defaultSetSelectionOptions(), AlignCursorOnScrollIfNeeded, CharacterGranularity);
>     m_previousCaretNode = nullptr;
>+    m_typingStyle = nullptr;
>+    m_appearanceUpdateTimer.stop();
> }
> 
> void FrameSelection::setStart(const VisiblePosition &pos, EUserTriggered trigger)
>Index: Source/WebCore/editing/FrameSelection.h
>===================================================================
>--- Source/WebCore/editing/FrameSelection.h	(revision 232420)
>+++ Source/WebCore/editing/FrameSelection.h	(working copy)
>@@ -341,7 +341,9 @@ private:
> 
>     RefPtr<EditingStyle> m_typingStyle;
> 
>+#if ENABLE(TEXT_CARET)
>     Timer m_caretBlinkTimer;
>+#endif
>     Timer m_appearanceUpdateTimer;
>     // The painted bounds of the caret in absolute coordinates
>     IntRect m_absCaretBounds;
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 232420)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-06-01  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Editor can hold references to Documents after you navigate away
>+        https://bugs.webkit.org/show_bug.cgi?id=186215
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added a regression test.
>+
>+        * editing/selection/navigation-clears-editor-state-expected.txt: Added.
>+        * editing/selection/navigation-clears-editor-state.html: Added.
>+
> 2018-06-01  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         ResourceLoader::cancel() shouldn't synchronously fire load event on document
>Index: LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt
>===================================================================
>--- LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt	(nonexistent)
>+++ LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt	(working copy)
>@@ -0,0 +1,12 @@
>+This tests navigating away from a document after setting a selection deletes the document.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS internals.numberOfLiveDocuments() is 1
>+PASS initialDocumentCount is 2
>+PASS internals.numberOfLiveDocuments() is 2
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>Index: LayoutTests/editing/selection/navigation-clears-editor-state.html
>===================================================================
>--- LayoutTests/editing/selection/navigation-clears-editor-state.html	(nonexistent)
>+++ LayoutTests/editing/selection/navigation-clears-editor-state.html	(working copy)
>@@ -0,0 +1,69 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="../../resources/js-test.js"></script>
>+<script>
>+
>+description('This tests navigating away from a document after setting a selection deletes the document.');
>+jsTestIsAsync = true;
>+
>+function appendIframe()
>+{
>+    const iframe = document.createElement('iframe');
>+    document.body.appendChild(iframe);
>+    iframe.contentDocument.body.innerHTML = '<p>hello</p>';
>+    return iframe;
>+}
>+
>+function selectAll(iframe)
>+{
>+    iframe.contentWindow.getSelection().selectAllChildren(iframe.contentDocument.body);
>+}
>+
>+function selectFirstPosition(iframe)
>+{
>+    iframe.contentWindow.getSelection().setPosition(iframe.contentDocument.body, 0);
>+}
>+
>+function wait(duration)
>+{
>+    return new Promise(function (resolve) {
>+        setTimeout(resolve, 0);
>+    });
>+}
>+
>+async function runTest()
>+{
>+    shouldBe('internals.numberOfLiveDocuments()', '1');
>+    const iframe = appendIframe();
>+
>+    await wait(0); // Make sure the transient document created by inserting an iframe is removed.
>+    GCController.collect();
>+
>+    initialDocumentCount = internals.numberOfLiveDocuments();
>+    shouldBe('initialDocumentCount', '2');
>+    selectAll(iframe);
>+
>+    await wait(0); // Wait for UI update timer to fire.
>+
>+    iframe.src = "resources/select-iframe-focusin-document-crash-frame.html";
>+    iframe.onload = () => {
>+        GCController.collect();
>+        shouldBe('internals.numberOfLiveDocuments()', '2');
>+        finishJSTest();                
>+    }
>+}
>+
>+if (!window.GCController || !window.internals) {
>+    debug('This test requires GCController and internals');
>+} else {
>+    // Clear out any lingering documents from the previous tests.
>+    GCController.collect();
>+    GCController.collect();
>+    runTest();
>+
>+}
>+
>+</script>
>+</body>
>+</html>

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186215: 341820 | 341821