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.
Created attachment 62659 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (bd8706ccf27f504a18a4e8df4cbfc99f)
Created attachment 62660 [details]
Repro - WebCore::ContainerNode::childNodeCount ReadAV@NULL (fb6d0e8b899470d4cd53d1586bb52dc3)
Created attachment 63866 [details]
I confirmed all three crashed are fixed by the patch.
Landed as r65061.
http://trac.webkit.org/changeset/65061 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug
GTK bots have been crashing on editing/selection/selection-modify-crash.html every run since this landed (5 so far).
Test is added to the skip list in http://trac.webkit.org/changeset/65078.
This bug seem to require another fix on gtk.
Um... Why are we skipping newly introduced crashers? That seems like the wrong approach. We should be fixing or rolling out, no?
(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.
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.
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.
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
(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
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.
(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;
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
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.
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.
The repros I attached to this bug no longer crash Chromium latest - time to close the bug?
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
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.