Bug 150670 – [AX] WebProcess from WebKitGtk+ 2.10.0 compiled in Debug mode hits ASSERT on textUnderElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gomez Garcia]

Created attachment 264315 [details]
BT from gdb

I'm using WebKitGtk+ with my own JHBuild setting:
https://github.com/tanty/jhbuild-epiphany/tree/wkgtk-devel

Epiphany 3.18.0 and WebKit 2.10.0

I'm running Epiphany with the dconf key:

"process-model" = "shared-secondary-process"

The compilation was done with CMake args:

"-DPORT=GTK -DCMAKE_BUILD_TYPE=Debug -DDEVELOPER_MODE=ON -DCMAKE_C_FLAGS_DEBUG=-g1 -DCMAKE_CXX_FLAGS_DEBUG=-g1"

When visiting several pages, eventually, WebKitWebProcess crashes.

This bug is not reproducible in a predictable way.

[Radar WebKit Bug Importer]

<rdar://problem/23314058>

[Andres Gomez Garcia]

Created attachment 264346 [details]
Another similar BT from gdb

This happened upon visiting http://www.as.com

[Andres Gomez Garcia]

Created attachment 264387 [details]
Yet another similar BT from gdb

This ASSERT is really easy to reproduce just by visiting http://www.as.com

[Mario Sanchez Prada]

I've tried to reproduce this with a debug build of the latest code from trunk using MiniBrowser (sorry, don't have a valid jhbuild-based setup now to try epiphany, sorry) and I could not reproduce the problem.

I thought it could be that perhaps the a11y tree was not properly initialized, but then I've checked with Orca and Accerciser and the tree is there, but I can't still get the crash by loading as.com, nor by reloading it after the a11y tree has been created.

I think it would be really helpful if you (or someone else) could try to get a reliable way to reproduce this in Minibrowser, if possible, as epiphany introduces a few more variables which would be great to isolate.

[Andres Gomez Garcia]

If we remove this ASSERT, then, we hit the next ASSERT at:

Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -1707,7 +1707,7 @@ String AccessibilityNodeObject::textUnderElement(AccessibilityTextUnderElementMo
     // TextIterator will force a layout update, potentially altering the accessibility tree
     // and leading to crashes in the loop that computes the result text from the children.
     // ASSERT(!document()->renderView()->layoutState());
     ASSERT(!document()->childNeedsStyleRecalc());

[Joanmarie Diggs]

Created attachment 264603 [details]
test case

I can reproduce this crash and came up with a greatly simplified test case (attached).

It seems like there's some craziness taking place that eventually works its way to the accessibility code. For instance:

1. Delete the space inside the 'script' element and the crash goes away and the text gets rendered.

2. Get rid of the float style in the first list item and the crash goes away and the text gets rendered.

3. Get rid of the text-transform style in the second list item and the crash goes away and the text gets rendered.

If you instead leave the attached test case as-is and view it in a non-debug build, the text gets rendered and you can use the accessible text interface to get the text for each list item element.

When I first started looking at this bug, my question was: Are the two asserts in question still needed? (The layout test associated with the addition of those assertions still passes when run with a debug build from which those assertions have been removed.) Now my question is WTF is going on, especially with the first item above (removing the space inside the 'script' element makes the crash go away).

[Joanmarie Diggs]

Sorry for the spam, but I just noticed that my attached test case doesn't trigger the first assertion; just the second. I'll make a new test case to trigger the first assertion later.

[Joanmarie Diggs]

Created attachment 264660 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 264660 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityList.cpp:130
> +        String text = axObjectCache()->getOrCreate(child)->stringValue();

Instead of navigating the child renderers here and then gettingOrCreating the axObject to call stringValue, I wonder if simply calling RenderText::text() (as long as child->isText() is true) would work.

I ask this because I'm concerned about firing the creation of a axObject here (that we won't expose via ATK anyway) simply to end up getting the text value from the associated renderer.

[Joanmarie Diggs]

(In reply to comment #9)
> Comment on attachment 264660 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264660&action=review
> 
> > Source/WebCore/accessibility/AccessibilityList.cpp:130
> > +        String text = axObjectCache()->getOrCreate(child)->stringValue();
> 
> Instead of navigating the child renderers here and then gettingOrCreating
> the axObject to call stringValue, I wonder if simply calling
> RenderText::text() (as long as child->isText() is true) would work.
> 
> I ask this because I'm concerned about firing the creation of a axObject
> here (that we won't expose via ATK anyway) simply to end up getting the text
> value from the associated renderer.

Understood. Any idea how much more costly that might be?

The reason I ask is that stringValue() calls textUnderElement() which in turn handles RenderTextFragment values including any associated altText set on those fragments.

I pondered pulling that code out into a new method which both stringValue() and childHasPseudoVisibleListItemMarkers() could use. But then I considered how infrequently this code would be called (hopefully there are not that many list markers with pseudo elements with tons of children out there) and couldn't be bothered. Would you like me to become sufficiently bothered? :)

Lemme know. And thanks for the review!

[Joanmarie Diggs]

*** Bug 147003 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

Created attachment 264790 [details]
Patch proposal

(In reply to comment #10)
> [...]
> > I ask this because I'm concerned about firing the creation of a axObject
> > here (that we won't expose via ATK anyway) simply to end up getting the text
> > value from the associated renderer.
> 
> Understood. Any idea how much more costly that might be?

Probably not too expensive, but kind of pointless since we are not going to wrap those objects in any way in ATK, AFAIK.

> The reason I ask is that stringValue() calls textUnderElement() which in
> turn handles RenderTextFragment values including any associated altText set
> on those fragments.
>
> I pondered pulling that code out into a new method which both stringValue()
> and childHasPseudoVisibleListItemMarkers() could use. But then I considered
> how infrequently this code would be called (hopefully there are not that
> many list markers with pseudo elements with tons of children out there) and
> couldn't be bothered. Would you like me to become sufficiently bothered? :)

This is a very good point, you're totally right on that my suggestion is a terrible oversimplification, as we can't simply ignore the different kind of text renderers we could have there...

But this point, together with our conversation yesterday on IRC, made me thing that the problem is that those ASSERTs, while still needed, are probably just in the wrong place, since they state a condition (stable render tree) that has too be fulfilled before using the TextIterator, but that won't ever happen at the level of AccessibilityNodeObject (where the ASSERTs currently live).

Instead, putting those ASSERTs in AccessibilityRenderObject::textUnderElement(), right before calling plainText(), is probably a much better place, so I cooked a patch to try that option which seems to work fine: does not hit the ASSERTs in the attached test case nor in your new layout test, and does not cause any regression to former tests.

So, as discussed on IRC, I'm attaching a new patch, keeping your newly added layout test.

Chris, as this is not ATK-specific code, it would be great if you could take a quick look. Thanks!

[chris fleizach]

Comment on attachment 264790 [details]
Patch proposal

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

> LayoutTests/accessibility/gtk/list-item-with-pseudo-element-crash.html:6
> +.item:before {content:""; }

Can we add this test for all platforms. Doesn't look like anything specific to gtk in the test

[Mario Sanchez Prada]

(In reply to comment #13)
> Comment on attachment 264790 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264790&action=review
> 
> > LayoutTests/accessibility/gtk/list-item-with-pseudo-element-crash.html:6
> > +.item:before {content:""; }
> 
> Can we add this test for all platforms. Doesn't look like anything specific
> to gtk in the test

Sure. I'll do that before landing. Thanks!

[Mario Sanchez Prada]

Committed r192022: <http://trac.webkit.org/changeset/192022>

[Ryan Haddad]

The layout test added with this patch is failing on all platforms:
<http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Flist-item-with-pseudo-element-crash.html>

[Ryan Haddad]

(In reply to comment #16)
> The layout test added with this patch is failing on all platforms:
> <http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=accessibility%2Flist-item-with-pseudo-element-
> crash.html>

--- /Volumes/Data/slave/mavericks-debug-tests-wk1/build/layout-test-results/accessibility/list-item-with-pseudo-element-crash-expected.txt
+++ /Volumes/Data/slave/mavericks-debug-tests-wk1/build/layout-test-results/accessibility/list-item-with-pseudo-element-crash-actual.txt
@@ -1,11 +1,8 @@
-Foo
-BAR
-This verifies that list items with pseudo elements and styles do not trigger an assertion.
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS successfullyParsed is true
-
-TEST COMPLETE
-
+CONSOLE MESSAGE: line 17: TypeError: description is not a function. (In 'description("This verifies that list items with pseudo elements and styles do not trigger an assertion.")', 'description' is an instance of HTMLParagraphElement)
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {P} at (0,0) size 784x0
+      RenderBlock {DIV} at (0,0) size 784x0

[Ryan Haddad]

Created attachment 264796 [details]
Patch

[Mario Sanchez Prada]

Stupid me. Forgot to update the path when moving the layout test one level up.

Ryan, feel free to commit your patch and re-resolving this again. Thanks!

[Ryan Haddad]

No worries! Committed in <http://trac.webkit.org/changeset/192026>

[Andres Gomez Garcia]

(In reply to comment #20)
> No worries! Committed in <http://trac.webkit.org/changeset/192026>

Thanks a lot, people! :)