Bug 161276 – AX: Crash at AccessibilityRenderObject::computeAccessibilityIsIgnored const + 552

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

[Nan Wang]

0   WebCore                       	0x0000000184b138b8 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 552 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/rendering/RenderObject.h:970)
1   WebCore                       	0x0000000184b13878 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 488 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/accessibility/AccessibilityRenderObject.cpp:1208)
2   WebCore                       	0x0000000184b0e0a0 WebCore::AccessibilityObject::accessibilityIsIgnored() const + 100 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/accessibility/AccessibilityObject.cpp:2748)
3   WebCore                       	0x0000000184b0db8c WebCore::AccessibilityObject::notifyIfIgnoredValueChanged() + 32 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/accessibility/AccessibilityObject.cpp:2643)
4   WebCore                       	0x00000001853ed240 WebCore::RenderBlock::removeChild(WebCore::RenderObject&) + 580 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/rendering/RenderBlock.cpp:768)
5   WebCore                       	0x00000001849bf924 WebCore::RenderObject::willBeDestroyed() + 72 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/rendering/RenderObject.cpp:191)
6   WebCore                       	0x00000001849bf6ec WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 276 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/rendering/RenderObject.cpp:2036)
7   WebCore                       	0x00000001855f8d24 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 288 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:324)
8   WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
9   WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
10  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
11  WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
12  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
13  WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
14  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
15  WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
16  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
17  WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
18  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
19  WebCore                       	0x00000001855f8ce8 WebCore::Style::detachChildren(WebCore::ContainerNode&, WebCore::Style::DetachType) + 228 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:543)
20  WebCore                       	0x00000001855f751c WebCore::Style::detachRenderTree(WebCore::Element&, WebCore::Style::DetachType) + 128 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/style/StyleResolveTree.cpp:570)
21  WebCore                       	0x0000000184bd3a74 WebCore::ContainerNode::removeBetween(WebCore::Node*, WebCore::Node*, WebCore::Node&) + 108 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/dom/ContainerNode.cpp:104)
22  WebCore                       	0x0000000184aa44a4 WebCore::ContainerNode::removeChild(WebCore::Node*, int&) + 336 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/dom/ContainerNode.cpp:572)
23  WebCore                       	0x0000000184aa434c WebCore::Node::removeChild(WebCore::Node*, int&) + 40 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/dom/Node.cpp:465)
24  WebCore                       	0x0000000184aa42e8 WebCore::JSNode::removeChild(JSC::ExecState*) + 68 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.1.46.128/bindings/js/JSNodeCustom.cpp:140)

<rdar://problem/22131009>

[Radar WebKit Bug Importer]

<rdar://problem/28039070>

[Nan Wang]

Created attachment 287168 [details]
Patch

[chris fleizach]

Comment on attachment 287168 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1158
> +    RenderObject* protectedRenderer(m_renderer);

does this hold onto the object? or just make a copy of the pointer?

[Nan Wang]

(In reply to comment #3)
> Comment on attachment 287168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287168&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1158
> > +    RenderObject* protectedRenderer(m_renderer);
> 
> does this hold onto the object? or just make a copy of the pointer?

 Good point, I'll double check. The RenderObject is not ref counted.

[Nan Wang]

Comment on attachment 287168 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1158
>>> +    RenderObject* protectedRenderer(m_renderer);
>> 
>> does this hold onto the object? or just make a copy of the pointer?
> 
> Good point, I'll double check. The RenderObject is not ref counted.

Since m_renderer is just a raw pointer, we have no way to know anything about the ownership and hold onto it easily (I did a lot of searches on smart pointers but I don't think we want to declare sole ownership within this function). A copy of the RenderObject might work but I suspect it will affect the performance quite a lot. The easiest way I can think of (since we are not sure what is causing the m_renderer being null) is to null check at all the place within this function when we are accessing m_renderer, but that seems to be very ugly.

[chris fleizach]

Maybe we can just do

RenderObject protect(mrenderer)

Then access it everywhere with

protect.isBR() and likewise with dot operator

(In reply to comment #5)
> Comment on attachment 287168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287168&action=review
> 
> >>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1158
> >>> +    RenderObject* protectedRenderer(m_renderer);
> >> 
> >> does this hold onto the object? or just make a copy of the pointer?
> > 
> > Good point, I'll double check. The RenderObject is not ref counted.
> 
> Since m_renderer is just a raw pointer, we have no way to know anything
> about the ownership and hold onto it easily (I did a lot of searches on
> smart pointers but I don't think we want to declare sole ownership within
> this function). A copy of the RenderObject might work but I suspect it will
> affect the performance quite a lot. The easiest way I can think of (since we
> are not sure what is causing the m_renderer being null) is to null check at
> all the place within this function when we are accessing m_renderer, but
> that seems to be very ugly.

[Nan Wang]

(In reply to comment #6)
> Maybe we can just do
> 
> RenderObject protect(mrenderer)
> 
> Then access it everywhere with
> 
> protect.isBR() and likewise with dot operator
> 

Why would this work? A reference is just an alia of the object and the object can still be nulled during this function, right? And C++ has undefined behavior on null reference, it will also cause crash?

[chris fleizach]

(In reply to comment #7)
> (In reply to comment #6)
> > Maybe we can just do
> > 
> > RenderObject protect(mrenderer)
> > 
> > Then access it everywhere with
> > 
> > protect.isBR() and likewise with dot operator
> > 
> 
> Why would this work? A reference is just an alia of the object and the
> object can still be nulled during this function, right? And C++ has
> undefined behavior on null reference, it will also cause crash?

I see a number of examples like this

   Ref<Element> protect(*element);

    bool successfullyFocused = newDocument->setFocusedElement(element, direction);

It seems however, if we do 
RenderObject protect(m_renderer)
and then access protect, as an object that would solve our problem. Our problem is now that the pointer inside m_renderer is bad, but rather what m_renderer points to. If we copy what m_renderer pointed to and use that for the duration of the method, it seems like that would work

[Nan Wang]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Maybe we can just do
> > > 
> > > RenderObject protect(mrenderer)
> > > 
> > > Then access it everywhere with
> > > 
> > > protect.isBR() and likewise with dot operator
> > > 
> > 
> > Why would this work? A reference is just an alia of the object and the
> > object can still be nulled during this function, right? And C++ has
> > undefined behavior on null reference, it will also cause crash?
> 
> I see a number of examples like this
> 
>    Ref<Element> protect(*element);
> 
>     bool successfullyFocused = newDocument->setFocusedElement(element,
> direction);
> 
> It seems however, if we do 
> RenderObject protect(m_renderer)
> and then access protect, as an object that would solve our problem. Our
> problem is now that the pointer inside m_renderer is bad, but rather what
> m_renderer points to. If we copy what m_renderer pointed to and use that for
> the duration of the method, it seems like that would work

I think for the other example, the class is ref counted so we can use Ref<T> to hold onto the object. 
Copying the object itself seems fine but would it affect the performance since we are calling this function everywhere? Also when I try to copy the object there's an compiler error saying RenderObject is an abstract class. Maybe we have to copy the derived class object? But that makes this problem more complicated.

[chris fleizach]

So our current protect does nothing then?

Can we not just copy the pointer which would take care of the nil pointer access

The other option is to put in more nil checks in some systemic way 

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Maybe we can just do
> > > > 
> > > > RenderObject protect(mrenderer)
> > > > 
> > > > Then access it everywhere with
> > > > 
> > > > protect.isBR() and likewise with dot operator
> > > > 
> > > 
> > > Why would this work? A reference is just an alia of the object and the
> > > object can still be nulled during this function, right? And C++ has
> > > undefined behavior on null reference, it will also cause crash?
> > 
> > I see a number of examples like this
> > 
> >    Ref<Element> protect(*element);
> > 
> >     bool successfullyFocused = newDocument->setFocusedElement(element,
> > direction);
> > 
> > It seems however, if we do 
> > RenderObject protect(m_renderer)
> > and then access protect, as an object that would solve our problem. Our
> > problem is now that the pointer inside m_renderer is bad, but rather what
> > m_renderer points to. If we copy what m_renderer pointed to and use that for
> > the duration of the method, it seems like that would work
> 
> I think for the other example, the class is ref counted so we can use Ref<T>
> to hold onto the object. 
> Copying the object itself seems fine but would it affect the performance
> since we are calling this function everywhere? Also when I try to copy the
> object there's an compiler error saying RenderObject is an abstract class.
> Maybe we have to copy the derived class object? But that makes this problem
> more complicated.

[Nan Wang]

(In reply to comment #10)
> So our current protect does nothing then?
> 
> Can we not just copy the pointer which would take care of the nil pointer
> access
> 
> The other option is to put in more nil checks in some systemic way 
> 

Yes in my understanding, the current protect is just copying the raw pointer and won't hold onto the object for longer duration.

[Nan Wang]

Created attachment 288647 [details]
patch

Added more nil checks.
Since we've only seen crashes at/before this line. Early return should fix most cases. That's why I didn't put nil checks for every m_renderer in the latter part of the function.

[chris fleizach]

Comment on attachment 288647 [details]
patch

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

> Source/WebCore/ChangeLog:14
> +        Despite my best efforts, I couldn't make a layout test that destroy the renderer within

that destroys the

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1193
> +    // <rdar://problem/22131009> Getting the controlObject might cause the m_renderer to be nullptr.

can you put webkit bug number here

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1215
> +        // Walking up the parents chain might reset the m_renderer

Walking up the parent chain

period at end of sentence

[Nan Wang]

Created attachment 288700 [details]
patch

update from review

[WebKit Commit Bot]

Comment on attachment 288700 [details]
patch

Clearing flags on attachment: 288700

Committed r205865: <http://trac.webkit.org/changeset/205865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.