Bug 43032 - Various "selection.modify" WebCore::ContainerNode::childNodeCount NULL pointer crashes
Summary: Various "selection.modify" WebCore::ContainerNode::childNodeCount NULL pointe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Kent Tamura
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-27 02:16 PDT by Berend-Jan Wever
Modified: 2010-09-29 10:51 PDT (History)
11 users (show)

See Also:


Attachments
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (73c50888d671e6db8bd5929edc78bd92) (543 bytes, text/html)
2010-07-27 02:16 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (bd8706ccf27f504a18a4e8df4cbfc99f) (570 bytes, text/html)
2010-07-27 02:18 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (fb6d0e8b899470d4cd53d1586bb52dc3) (335 bytes, text/html)
2010-07-27 02:18 PDT, Berend-Jan Wever
no flags Details
Patch (3.55 KB, patch)
2010-08-09 00:53 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2010-07-27 02:16:20 PDT
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.
Comment 1 Berend-Jan Wever 2010-07-27 02:18:19 PDT
Created attachment 62659 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (bd8706ccf27f504a18a4e8df4cbfc99f)
Comment 2 Berend-Jan Wever 2010-07-27 02:18:43 PDT
Created attachment 62660 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (fb6d0e8b899470d4cd53d1586bb52dc3)
Comment 3 Kent Tamura 2010-08-09 00:53:32 PDT
Created attachment 63866 [details]
Patch
Comment 4 Kent Tamura 2010-08-09 00:54:20 PDT
I confirmed all three crashed are fixed by the patch.
Comment 5 Kent Tamura 2010-08-10 02:48:45 PDT
Landed as r65061.
Comment 6 WebKit Review Bot 2010-08-10 04:13:14 PDT
http://trac.webkit.org/changeset/65061 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug
Comment 7 James Robinson 2010-08-10 10:34:48 PDT
GTK bots have been crashing on editing/selection/selection-modify-crash.html every run since this landed (5 so far).
Comment 8 Ryosuke Niwa 2010-08-10 11:23:57 PDT
Test is added to the skip list in http://trac.webkit.org/changeset/65078.
Comment 9 Ryosuke Niwa 2010-08-10 11:24:42 PDT
This bug seem to require another fix on gtk.
Comment 10 Eric Seidel (no email) 2010-08-10 11:25:26 PDT
Um... Why are we skipping newly introduced crashers?  That seems like the wrong approach. We should be fixing or rolling out, no?
Comment 11 Tony Chang 2010-08-10 11:28:03 PDT
(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.
Comment 12 Eric Seidel (no email) 2010-08-10 11:31:23 PDT
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.
Comment 13 James Robinson 2010-08-10 11:36:09 PDT
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.
Comment 14 Ryosuke Niwa 2010-08-10 13:48:04 PDT
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
Comment 15 Xan Lopez 2010-08-10 13:52:45 PDT
(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.
Comment 16 Mario Sanchez Prada 2010-08-10 23:55:37 PDT
(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
Comment 17 Martin Robinson 2010-08-11 14:34:12 PDT
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.
Comment 18 Kent Tamura 2010-08-11 18:41:13 PDT
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.
Comment 19 Berend-Jan Wever 2010-09-29 09:17:27 PDT
The repros I attached to this bug no longer crash Chromium latest - time to close the bug?
Comment 20 Martin Robinson 2010-09-29 09:43:29 PDT
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
Comment 21 Martin Robinson 2010-09-29 10:51:20 PDT
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.