Attachment 264933 Details for Bug 150944 – Patch proposal

[patch] Patch proposal

0001-2015-11-06-Mario-Sanchez-Prada-mario-endlessm.com.patch (text/plain), 6.66 KB, created by Mario Sanchez Prada on 2015-11-06 03:56:23 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mario Sanchez Prada

Created: 2015-11-06 03:56:23 PST

Size: 6.66 KB

patch

obsolete

>From 10c4ab63e2e65701b9e66af6795e79c00a99b1ff Mon Sep 17 00:00:00 2001
>From: Mario Sanchez Prada <mario@webkit.org>
>Date: Fri, 6 Nov 2015 11:15:31 +0000
>Subject: [PATCH] 2015-11-06  Mario Sanchez Prada  <mario@endlessm.com>
>
>        Layout Test accessibility/win/linked-elements.html is crashing on win debug
>        https://bugs.webkit.org/show_bug.cgi?id=150944
>
>        Reviewed by NOBODY (OOPS!).
>
>        Be more precise ASSERTing on textUnderElement, only checking that the render
>        tree is stable before using TextIteraror when in 'IncludeAllChildren' mode.
>
>        * accessibility/AccessibilityRenderObject.cpp:
>        (WebCore::AccessibilityRenderObject::textUnderElement):
>
>2015-11-06  Mario Sanchez Prada  <mario@endlessm.com>
>
>        Layout Test accessibility/win/linked-elements.html is crashing on win debug
>        https://bugs.webkit.org/show_bug.cgi?id=150944
>
>        Reviewed by NOBODY (OOPS!).
>
>        Removed accessibility/win/linked-elements.html crashing expectation.
>
>        * platform/win/TestExpectations: Removed crashing expectation.
>---
> LayoutTests/ChangeLog                                    | 11 +++++++++++
> LayoutTests/platform/win/TestExpectations                |  2 --
> Source/WebCore/ChangeLog                                 | 13 +++++++++++++
> .../WebCore/accessibility/AccessibilityRenderObject.cpp  | 16 +++++++++-------
> 4 files changed, 33 insertions(+), 9 deletions(-)
>
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ad086de..ad68c27 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2015-11-06  Mario Sanchez Prada  <mario@endlessm.com>
>+
>+        Layout Test accessibility/win/linked-elements.html is crashing on win debug
>+        https://bugs.webkit.org/show_bug.cgi?id=150944
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Removed accessibility/win/linked-elements.html crashing expectation.
>+
>+        * platform/win/TestExpectations: Removed crashing expectation.
>+
> 2015-11-05  Sukolsak Sakshuwong  <sukolsak@gmail.com>
> 
>         Layout Test js/intl-collator.html is crashing on win 7 debug
>diff --git a/LayoutTests/platform/win/TestExpectations b/LayoutTests/platform/win/TestExpectations
>index 0003eb6..aa4debb 100644
>--- a/LayoutTests/platform/win/TestExpectations
>+++ b/LayoutTests/platform/win/TestExpectations
>@@ -3305,8 +3305,6 @@ webkit.org/b/150808 fast/text/multiple-feature-properties.html [ ImageOnlyFailur
> 
> webkit.org/b/147933 streams/reference-implementation/writable-stream-abort.html [ Pass Failure ]
> 
>-webkit.org/b/150944 [ Debug ] accessibility/win/linked-elements.html [ Crash ]
>-
> webkit.org/b/150946 [ Debug ] scrollbars/custom-scrollbar-appearance-property.html [ Crash ]
> 
> webkit.org/b/150948 [ Debug ] imported/blink/transitions/unprefixed-transform.html [ Pass Failure ]
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index daeac15..acf6b27 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,16 @@
>+2015-11-06  Mario Sanchez Prada  <mario@endlessm.com>
>+
>+        Layout Test accessibility/win/linked-elements.html is crashing on win debug
>+        https://bugs.webkit.org/show_bug.cgi?id=150944
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Be more precise ASSERTing on textUnderElement, only checking that the render
>+        tree is stable before using TextIteraror when in 'IncludeAllChildren' mode.
>+
>+        * accessibility/AccessibilityRenderObject.cpp:
>+        (WebCore::AccessibilityRenderObject::textUnderElement):
>+
> 2015-11-06  Yoav Weiss  <yoav@yoav.ws>
> 
>         Expose HTMLImageElement sizes attribute in IDL
>diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>index da8b2bf..1ebf7c8 100644
>--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>@@ -632,10 +632,12 @@ String AccessibilityRenderObject::textUnderElement(AccessibilityTextUnderElement
>     if (m_renderer->isBR())
>         return ASCIILiteral("\n");
> 
>+    bool isRenderText = is<RenderText>(*m_renderer);
>+
> #if ENABLE(MATHML)
>     // Math operators create RenderText nodes on the fly that are not tied into the DOM in a reasonable way,
>     // so rangeOfContents does not work for them (nor does regular text selection).
>-    if (is<RenderText>(*m_renderer) && m_renderer->isAnonymous() && ancestorsOfType<RenderMathMLOperator>(*m_renderer).first())
>+    if (isRenderText && m_renderer->isAnonymous() && ancestorsOfType<RenderMathMLOperator>(*m_renderer).first())
>         return downcast<RenderText>(*m_renderer).text();
>     if (is<RenderMathMLOperator>(*m_renderer) && !m_renderer->isAnonymous())
>         return downcast<RenderMathMLOperator>(*m_renderer).element().textContent();
>@@ -647,7 +649,8 @@ String AccessibilityRenderObject::textUnderElement(AccessibilityTextUnderElement
> 
>     // We use a text iterator for text objects AND for those cases where we are
>     // explicitly asking for the full text under a given element.
>-    if (is<RenderText>(*m_renderer) || mode.childrenInclusion == AccessibilityTextUnderElementMode::TextUnderElementModeIncludeAllChildren) {
>+    bool shouldIncludeAllChildren = mode.childrenInclusion == AccessibilityTextUnderElementMode::TextUnderElementModeIncludeAllChildren;
>+    if (isRenderText || shouldIncludeAllChildren) {
>         // If possible, use a text iterator to get the text, so that whitespace
>         // is handled consistently.
>         Document* nodeDocument = nullptr;
>@@ -687,11 +690,10 @@ String AccessibilityRenderObject::textUnderElement(AccessibilityTextUnderElement
>                 if (frame->document() != nodeDocument)
>                     return String();
> 
>-                // The render tree should be stable before going ahead. Otherwise, further uses of the
>-                // 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(!nodeDocument->renderView()->layoutState());
>-                ASSERT(!nodeDocument->childNeedsStyleRecalc());
>+                // The tree should be stable before looking through the children of a non-Render Text object.
>+                // Otherwise, further uses of TextIterator will force a layout update, potentially altering
>+                // the accessibility tree and causing crashes in the loop that computes the result text.
>+                ASSERT((isRenderText || !shouldIncludeAllChildren) || (!nodeDocument->renderView()->layoutState() && !nodeDocument->childNeedsStyleRecalc()));
> 
>                 return plainText(textRange.get(), textIteratorBehaviorForTextRange());
>             }
>-- 
>2.4.3
>

Flags:

cfleizach: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 150944: 264933