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>
<rdar://problem/28039070>
Created attachment 287168 [details] Patch
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?
(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.
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.
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.
(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?
(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
(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.
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.
(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.
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.
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
Created attachment 288700 [details] patch update from review
Comment on attachment 288700 [details] patch Clearing flags on attachment: 288700 Committed r205865: <http://trac.webkit.org/changeset/205865>
All reviewed patches have been landed. Closing bug.