Bug 43032 – Various "selection.modify" WebCore::ContainerNode::childNodeCount NULL pointer crashes

• Component: HTML Editing
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Created attachment 62658 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (73c50888d671e6db8bd5929edc78bd92)

Attached is a repro for a NULL pointer crash in WebCore::ContainerNode::childNodeCount. I'll add a few more related crashes. It appears this bug can only be used to crash WebKit by causing a NULL pointer exception.

[Berend-Jan Wever]

Created attachment 62659 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (bd8706ccf27f504a18a4e8df4cbfc99f)

[Berend-Jan Wever]

Created attachment 62660 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (fb6d0e8b899470d4cd53d1586bb52dc3)

[Kent Tamura]

Created attachment 63866 [details]
Patch

[Kent Tamura]

I confirmed all three crashed are fixed by the patch.

[Kent Tamura]

Landed as r65061.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65061 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug

[James Robinson]

GTK bots have been crashing on editing/selection/selection-modify-crash.html every run since this landed (5 so far).

[Ryosuke Niwa]

Test is added to the skip list in http://trac.webkit.org/changeset/65078.

[Ryosuke Niwa]

This bug seem to require another fix on gtk.

[Eric Seidel (no email)]

Um... Why are we skipping newly introduced crashers?  That seems like the wrong approach. We should be fixing or rolling out, no?

[Tony Chang]

(In reply to comment #10)
> Um... Why are we skipping newly introduced crashers?  That seems like the wrong approach. We should be fixing or rolling out, no?

I made the suggestion to skip on GTK because it passes on other platforms.  I suspect that this is an existing crash in WebKit GTK that the new test is just exposing, but without a GTK build handy, I'm not certain.

If you think it's better to roll out the original crash fix, I'm not opposed.

[Eric Seidel (no email)]

Just so long as it's not a permenant fix. :)  It sounds like from the comment which mid-air colided with mine, that rniwa is working on fixing it as we speak.  So I'm fine with leaving the fix in.

We've often found that when we land a patch and it fails mysteriously on gtk or qt, that there were real bugs in the patch. :)

Xan or Martin should be able to hook you up with some gtk backtraces.  Or you can ask on #webkit-gtk.

Martin works in SF and generally super available and super helpful for this kind of thing.

[James Robinson]

The test that is crashing is new.  The patch didn't cause the crash, the test exposes a crash that (presumably) has always been there.  Rolling out the patch would only remove the test case, it wouldn't fix anything.

[Ryosuke Niwa]

It seems like the corresponding crash log is http://webkit-bots.igalia.com/amd64/svn_65077.core-when_1281463861-_-who_DumpRenderTree-_-why_11.23383.trace.html

+msanchez

[Xan Lopez]

(In reply to comment #14)
> It seems like the corresponding crash log is http://webkit-bots.igalia.com/amd64/svn_65077.core-when_1281463861-_-who_DumpRenderTree-_-why_11.23383.trace.html
> 
> +msanchez

From the trace it seems the likely cause is that we are getting a parentObject that is NULL. My poor understanding of the topic tells me that this shouldn't happen (there should always be a root object), so this would be exposing a deeper issue I suppose. If on the other hand getting a NULL parent makes sense in some cases (and for this test) then the fix is simple.

[Mario Sanchez Prada]

(In reply to comment #15)
> [...]
> From the trace it seems the likely cause is that we are getting a parentObject 
> that is NULL. My poor understanding of the topic tells me that this shouldn't 
> happen (there should always be a root object), so this would be exposing a 
> deeper issue I suppose. If on the other hand getting a NULL parent makes sense 
> in some cases (and for this test) then the fix is simple.

+1 to Xan's reasoning. Now trying to throw some additional info over the table...

AFAIK, it's strange you get a null parentObject unless you directly asked for it to the root #document object, since calls to parentObject() or parentObjectUnignored() will always return a valid AccessibleObject (the one associated with the #document node, in the most xtreme case). 

But, in AccessibilytObjectWrapper.cpp:

AccessibilityObject* objectAndOffsetUnignored(AccessibilityObject* coreObject, int& offset, bool ignoreLinks)
{
    Node* endNode = static_cast<AccessibilityRenderObject*>(coreObject)->renderer()->node();
    int endOffset = coreObject->selection().end().computeOffsetInContainerNode();
    // Indication that something bogus has transpired.
    offset = -1;

    AccessibilityObject* realObject = coreObject;
    if (realObject->accessibilityIsIgnored())
        realObject = realObject->parentObjectUnignored();

    if (ignoreLinks && realObject->isLink()) <------- Guilty line
        realObject = realObject->parentObjectUnignored();

    [...]
}

... realObject can be either the coreObject (which is not null, as seen in the stacktrace) or the parent for the original coreObject *in case it did ignore accessibility*, which is not the case of the AccessibleObject associated to the #document node (the only one which in theory could return NULL through parentObject() or parentObjectUnignored()), thus that never could happen in theory.

However, seeing that it crashes on that line but not in the previous ones calling to methods of coreObject and/or realObject looks to me like the confirmation of the "realObject = realObject->parentObjectUnignored()" line being executed and somehow returning NULL so I think there's definitely something wrong in there (perhaps there's no AccessibleObject for #document object -or someone else- created yet, so the a11y hierarchy is somehow broken?).

Btw, any advice/tip on how to reliably reproduce this? I'd like to take a closer look to this issue to try to help as much as possible

[Martin Robinson]

Mario, have you been able to reproduce this issue by running the test that failed? I think the Chromium guys may have the information regarding this.

[Kent Tamura]

I couldn't reproduce the crash with Ubuntu 9.10, GTK port, amd64, Debug and Release.

Mario, Martin, LayoutTests/editing/selection/selection-modify-crash.html should reproduce the crash though I couldn't.

[Berend-Jan Wever]

The repros I attached to this bug no longer crash Chromium latest - time to close the bug?

[Martin Robinson]

It looks like this crash is only reproducible when run via Xvfb. Instructions for running via Xvfb are here: https://trac.webkit.org/wiki/WebKitGtkLayoutTests

[Martin Robinson]

The crasher on GTK+ is unrelated to this issue. I've posted a patch for that here: https://bugs.webkit.org/show_bug.cgi?id=46822 Sorry, that this has been delayed so long.