Bug 186215 – Editor can hold references to Documents after you navigate away

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

If a page triggers Editor VisibleSelection code—say, like:

            var selection = window.getSelection();
            var container = document.getElementById("container");
            selection.setPosition(container, 0);

— then Frame’s m_editor retains that Document after you navigate away from the page. It’s only released when the Frame is released:

  * frame #0: 0x000000010d3b4c5b WebCore`WebCore::Document::~Document(this=0x000000012ad00fb8) at Document.cpp:582
    frame #1: 0x000000010d757e55 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x000000012ad00fb8) at HTMLDocument.cpp:95
    frame #2: 0x000000010d757e75 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x000000012ad00fb8) at HTMLDocument.cpp:95
    frame #3: 0x000000010d757f19 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x000000012ad00fb8) at HTMLDocument.cpp:95
    frame #4: 0x000000010d3b8590 WebCore`WebCore::Document::decrementReferencingNodeCount(this=0x000000012ad00fb8) at Document.h:361
    frame #5: 0x000000010d4e80b0 WebCore`WebCore::Node::~Node(this=0x000000012b400528) at Node.cpp:314
    frame #6: 0x000000010d38139d WebCore`WebCore::CharacterData::~CharacterData(this=0x000000012b400528) at CharacterData.h:29
    frame #7: 0x000000010d586135 WebCore`WebCore::Text::~Text(this=0x000000012b400528) at Text.cpp:56
    frame #8: 0x000000010d586155 WebCore`WebCore::Text::~Text(this=0x000000012b400528) at Text.cpp:56
    frame #9: 0x000000010d586179 WebCore`WebCore::Text::~Text(this=0x000000012b400528) at Text.cpp:56
    frame #10: 0x000000010d4e87fb WebCore`WebCore::Node::removedLastRef(this=0x000000012b400528) at Node.cpp:2557
    frame #11: 0x000000010d4e876c WebCore`WebCore::Node::deref(this=0x000000012b400528) at Node.cpp:365
    frame #12: 0x000000010b7b879e WebCore`void WTF::derefIfNotNull<WebCore::Node>(ptr=0x000000012b400528) at RefPtr.h:45
    frame #13: 0x000000010b7b8769 WebCore`WTF::RefPtr<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >::~RefPtr(this=0x0000000124cf2900) at RefPtr.h:70
    frame #14: 0x000000010b7b7c05 WebCore`WTF::RefPtr<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >::~RefPtr(this=0x0000000124cf2900) at RefPtr.h:70
    frame #15: 0x000000010b984bd5 WebCore`WebCore::Position::~Position(this=0x0000000124cf2900) at Position.h:55
    frame #16: 0x000000010b984bb5 WebCore`WebCore::Position::~Position(this=0x0000000124cf2900) at Position.h:55
    frame #17: 0x000000010ba7a3cc WebCore`WebCore::VisibleSelection::~VisibleSelection(this=0x0000000124cf2900) at VisibleSelection.h:38
    frame #18: 0x000000010ba75f65 WebCore`WebCore::VisibleSelection::~VisibleSelection(this=0x0000000124cf2900) at VisibleSelection.h:38
    frame #19: 0x000000010d6122a0 WebCore`WebCore::Editor::~Editor(this=0x0000000124cf2800) at Editor.cpp:1164
    frame #20: 0x000000010d612635 WebCore`WebCore::Editor::~Editor(this=0x0000000124cf2800) at Editor.cpp:1164
    frame #21: 0x000000010dd966eb WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef() [inlined] std::__1::default_delete<WebCore::Editor>::operator(this=0x0000000124cb5308, __ptr=0x0000000124cf2800)(WebCore::Editor*) const at memory:2239
    frame #22: 0x000000010dd966d0 WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef() [inlined] std::__1::unique_ptr<WebCore::Editor, std::__1::default_delete<WebCore::Editor> >::reset(this=0x0000000124cb5308, __p=0x0000000000000000) at memory:2552
    frame #23: 0x000000010dd96683 WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef() [inlined] std::__1::unique_ptr<WebCore::Editor, std::__1::default_delete<WebCore::Editor> >::~unique_ptr(this=0x0000000124cb5308) at memory:2506
    frame #24: 0x000000010dd96683 WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef() [inlined] std::__1::unique_ptr<WebCore::Editor, std::__1::default_delete<WebCore::Editor> >::~unique_ptr(this=0x0000000124cb5308) at memory:2506
    frame #25: 0x000000010dd96683 WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef(this=0x0000000124cb5308) at UniqueRef.h:42
    frame #26: 0x000000010dd70045 WebCore`WTF::UniqueRef<WebCore::Editor>::~UniqueRef(this=0x0000000124cb5308) at UniqueRef.h:42
    frame #27: 0x000000010dd6fb86 WebCore`WebCore::Frame::~Frame(this=0x0000000124cb5230) at Frame.cpp:231
    frame #28: 0x000000010dd70125 WebCore`WebCore::Frame::~Frame(this=0x0000000124cb5230) at Frame.cpp:214
    frame #29: 0x000000010dd70149 WebCore`WebCore::Frame::~Frame(this=0x0000000124cb5230) at Frame.cpp:214
    frame #30: 0x000000010b9e26cf WebCore`WTF::ThreadSafeRefCounted<WebCore::AbstractFrame, (WTF::DestructionThread)0>::deref(this=0x0000000124cb5238) const at ThreadSafeRefCounted.h:76
    frame #31: 0x000000010b9e2653 WebCore`WTF::Ref<WebCore::Frame, WTF::DumbPtrTraits<WebCore::Frame> >::~Ref(this=0x0000000124dfe098) at Ref.h:61
    frame #32: 0x000000010b9e2465 WebCore`WTF::Ref<WebCore::Frame, WTF::DumbPtrTraits<WebCore::Frame> >::~Ref(this=0x0000000124dfe098) at Ref.h:55
    frame #33: 0x000000010ddc3a13 WebCore`WebCore::Page::~Page(this=0x0000000124dfe000) at Page.cpp:337
    frame #34: 0x000000010ddc4ad5 WebCore`WebCore::Page::~Page(this=0x0000000124dfe000) at Page.cpp:293

Seen with LayoutTests/fast/css/counters/counter-before-content-not-incremented.html when testing with changes in bug 186214.

[Simon Fraser (smfr)]

rdar://problem/40736891

[Simon Fraser (smfr)]

Also need to clean up Frame::selection()

[Ryosuke Niwa]

Created attachment 341820 [details]
Fixes the leak

[Ryosuke Niwa]

Created attachment 341821 [details]
Fixes the leak

[Simon Fraser (smfr)]

Comment on attachment 341821 [details]
Fixes the leak

View in context: https://bugs.webkit.org/attachment.cgi?id=341821&action=review

> Source/WebCore/editing/Editor.cpp:1178
> +    m_mark = VisibleSelection();
> +    m_oldSelectionForEditorUIUpdate = VisibleSelection();

I think these can be foo = { };

[Ryosuke Niwa]

Committed r232434: <https://trac.webkit.org/changeset/232434>

[Saam Barati]

Hi Ryosuke, I'm trying to debug this test with a patch I'm working on and finding some interesting results. I'd like your feedback. The patch I'm working on makes our conservative stack scan a bit more conservative: it'll keep alive interior pointers to JSCells.

I did some experimenting with your test, and have found the following results.

I rolled out your change that you landed, and confirmed that this test fails with your change rolled out, and passes with your change rolled in. (This is a variant of your original test that's identical semantically in my understanding):
```
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 setEditorStates(iframe)
{
    iframe.contentDocument.designMode = 'on';
    iframe.contentWindow.getSelection().setPosition(iframe.contentDocument.body, 1);
    iframe.contentDocument.execCommand('bold', false, null);
}

function wait(duration)
{
    return new Promise(function (resolve) {
        setTimeout(resolve, 0);
    });
}

var frame;
var initialDocumentCount;
async function runTest()
{
    let iframe = appendIframe();

    await wait(0); // Make sure the transient document created by inserting an iframe is removed.
    GCController.collect();

    setEditorStates(iframe);

    await wait(0); // Wait for UI update timer to fire.

    initialDocumentCount = internals.numberOfLiveDocuments();
    debug(`num live documents before setting src: ${internals.numberOfLiveDocuments()}`);
    iframe.src = "resources/select-iframe-focusin-document-crash-frame.html";
    let resolve;
    let p = new Promise(function (r) { resolve = r; });
    iframe.onload = () => {
        GCController.collect();
        resolve(internals.numberOfLiveDocuments() === initialDocumentCount);
    }
    let result = await p;
    return result;
}

async function runGC() {
    await wait(0);
    GCController.collect();
}

async function loop() {
    let success = false;
    for (let i = 0; i < 1; ++i) {
        let result = await runTest();
        if (result) {
            success = true;
            break;
        }
    }

    if (success)
        debug("Success!");
    else
        debug("Fail!");

    finishJSTest();                
}

if (!window.GCController || !window.internals) {
    debug('This test requires GCController and internals');
} else {
    if (window.testRunner)
        setTimeout(() => testRunner.notifyDone(), 3000);
    // Clear out any lingering documents from the previous tests.
    GCController.collect();
    GCController.collect();

    loop();
}
```


However, I modified this test slightly, to do the GC and document count check after a few ticks of the runloop, and this test passes both with our without your change in this patch:

```
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 setEditorStates(iframe)
{
    iframe.contentDocument.designMode = 'on';
    iframe.contentWindow.getSelection().setPosition(iframe.contentDocument.body, 1);
    iframe.contentDocument.execCommand('bold', false, null);
}

function wait(duration)
{
    return new Promise(function (resolve) {
        setTimeout(resolve, 0);
    });
}

var frame;
var initialDocumentCount;
async function runTest()
{
    let iframe = appendIframe();

    await wait(0); // Make sure the transient document created by inserting an iframe is removed.
    GCController.collect();

    setEditorStates(iframe);

    await wait(0); // Wait for UI update timer to fire.

    initialDocumentCount = internals.numberOfLiveDocuments();
    debug(`num live documents before setting src: ${internals.numberOfLiveDocuments()}`);
    iframe.src = "resources/select-iframe-focusin-document-crash-frame.html";
    let resolve;
    let p = new Promise(function (r) { resolve = r; });
    iframe.onload = () => {
        resolve();
    }
    let result = await p;
    return result;
}

async function runGC() {
    await wait(0);
    GCController.collect();
}

async function loop() {
    let success = false;
    for (let i = 0; i < 1; ++i) {
        await runTest();
        await runGC();
        if (initialDocumentCount === internals.numberOfLiveDocuments()) {
            success = true;
            break;
        }
    }

    if (success)
        debug("Success!");
    else
        debug("Fail!");

    finishJSTest();                
}

if (!window.GCController || !window.internals) {
    debug('This test requires GCController and internals');
} else {
    if (window.testRunner)
        setTimeout(() => testRunner.notifyDone(), 3000);
    // Clear out any lingering documents from the previous tests.
    GCController.collect();
    GCController.collect();

    loop();
}
```

So, I think there are at least a few things going on here (and probably more):
- Your original test is susceptible to spurious failure by having random stack values point to cells. As a matter of policy, we don't want tests like this, because they will eventually fail, because our conservative GC makes tests like this fail for a number of random reasons.
- The test itself may not be measuring exactly what you think it is, since doing the GC a few runloop ticks later will clean up the extra document. This is just a hypothesis, but I think you'll be able to analyze this better since you understand what the code is doing better than I do. I think the implication here might be that we either don't really need your change to clean up the document, or your change may help GCs happening in some more specific call stacks and not in the more general case.

Based on Simon's suggestion, I verified that the last document is indeed killed by the last GC in the above example. Based on your original description of the bug, I would have expected that not to happen. Based on these findings, I'm probably going to mark this test as flaky or delete it in my GC crasher fix.

Do you have any thoughts on this data/test?