Bug 150670 - [AX] WebProcess from WebKitGtk+ 2.10.0 compiled in Debug mode hits ASSERT on textUnderElement
Summary: [AX] WebProcess from WebKitGtk+ 2.10.0 compiled in Debug mode hits ASSERT on ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords: InRadar
: 147003 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-10-29 07:39 PDT by Andres Gomez Garcia
Modified: 2015-11-06 01:52 PST (History)
16 users (show)

See Also:


Attachments
BT from gdb (30.77 KB, text/plain)
2015-10-29 07:39 PDT, Andres Gomez Garcia
no flags Details
Another similar BT from gdb (27.62 KB, text/plain)
2015-10-29 14:34 PDT, Andres Gomez Garcia
no flags Details
Yet another similar BT from gdb (42.16 KB, text/plain)
2015-10-30 02:37 PDT, Andres Gomez Garcia
no flags Details
test case (236 bytes, text/html)
2015-11-02 11:25 PST, Joanmarie Diggs (irc: joanie)
no flags Details
Patch (5.67 KB, patch)
2015-11-02 20:44 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch proposal (7.74 KB, patch)
2015-11-04 06:20 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2015-11-04 10:19 PST, Ryan Haddad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gomez Garcia 2015-10-29 07:39:10 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-10-29 07:56:51 PDT
<rdar://problem/23314058>
Comment 2 Andres Gomez Garcia 2015-10-29 14:34:34 PDT
Created attachment 264346 [details]
Another similar BT from gdb

This happened upon visiting http://www.as.com
Comment 3 Andres Gomez Garcia 2015-10-30 02:37:50 PDT
Created attachment 264387 [details]
Yet another similar BT from gdb

This ASSERT is really easy to reproduce just by visiting http://www.as.com
Comment 4 Mario Sanchez Prada 2015-10-30 04:21:12 PDT
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.
Comment 5 Andres Gomez Garcia 2015-10-30 08:17:53 PDT
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());
Comment 6 Joanmarie Diggs (irc: joanie) 2015-11-02 11:25:16 PST
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).
Comment 7 Joanmarie Diggs (irc: joanie) 2015-11-02 11:45:26 PST
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.
Comment 8 Joanmarie Diggs (irc: joanie) 2015-11-02 20:44:10 PST
Created attachment 264660 [details]
Patch
Comment 9 Mario Sanchez Prada 2015-11-03 03:13:15 PST
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.
Comment 10 Joanmarie Diggs (irc: joanie) 2015-11-03 06:34:43 PST
(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!
Comment 11 Joanmarie Diggs (irc: joanie) 2015-11-03 09:18:49 PST
*** Bug 147003 has been marked as a duplicate of this bug. ***
Comment 12 Mario Sanchez Prada 2015-11-04 06:20:15 PST
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!
Comment 13 chris fleizach 2015-11-04 06:36:28 PST
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
Comment 14 Mario Sanchez Prada 2015-11-04 06:43:29 PST
(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!
Comment 15 Mario Sanchez Prada 2015-11-04 06:47:19 PST
Committed r192022: <http://trac.webkit.org/changeset/192022>
Comment 17 Ryan Haddad 2015-11-04 10:13:05 PST
(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
Comment 18 Ryan Haddad 2015-11-04 10:19:41 PST
Created attachment 264796 [details]
Patch
Comment 19 Mario Sanchez Prada 2015-11-04 10:41:50 PST
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!
Comment 20 Ryan Haddad 2015-11-04 10:43:01 PST
No worries! Committed in <http://trac.webkit.org/changeset/192026>
Comment 21 Andres Gomez Garcia 2015-11-06 01:52:20 PST
(In reply to comment #20)
> No worries! Committed in <http://trac.webkit.org/changeset/192026>

Thanks a lot, people! :)