Bug 209251 – Nullptr crash in RenderObject::RenderObjectBitfields::isBox when current renderer is the RenderView

• Component: Accessibility
• Status: RESOLVED FIXED

[Jack]

<rdar://60103614>

#0 0x60cb897fe in WebCore::RenderObject::RenderObjectBitfields::isBox() const+0x1e (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c67fe)
    #1 0x60cb897c4 in WebCore::RenderObject::isText() const+0x14 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c67c4)
    #2 0x60cb897a8 in WebCore::RenderObject::isRenderElement() const+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c67a8)
    #3 0x60cb89798 in WTF::TypeCastTraits<WebCore::RenderElement const, WebCore::RenderObject const, false>::isType(WebCore::RenderObject const&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c6798)
    #4 0x60cb89788 in WTF::TypeCastTraits<WebCore::RenderElement const, WebCore::RenderObject const, false>::isOfType(WebCore::RenderObject const&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c6788)
    #5 0x60cb8b178 in bool WTF::is<WebCore::RenderElement, WebCore::RenderObject const>(WebCore::RenderObject const&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x1c8178)
    #6 0x60f419fed in WebCore::RenderObject::isRenderInline() const+0xd (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a56fed)
    #7 0x60f420ae8 in WTF::TypeCastTraits<WebCore::RenderInline const, WebCore::RenderObject const, false>::isType(WebCore::RenderObject const&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a5dae8)
    #8 0x60f420ad8 in WTF::TypeCastTraits<WebCore::RenderInline const, WebCore::RenderObject const, false>::isOfType(WebCore::RenderObject const&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a5dad8)
    #9 0x60f401ed8 in bool WTF::is<WebCore::RenderInline, WebCore::RenderObject>(WebCore::RenderObject&)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a3eed8)
    #10 0x60f3ebd3d in WebCore::isInlineWithContinuation(WebCore::RenderObject&)+0xd (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a28d3d)
    #11 0x60f3eb9f4 in WebCore::AccessibilityRenderObject::nextSibling() const+0x144 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a289f4)
    #12 0x60f3cabea in WebCore::AccessibilityObject::nextSiblingUnignored(int) const+0x4a (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x2a07bea)
    #13 0x60f35c6fa in WebCore::AXObjectCache::postTextStateChangeNotification(WebCore::Position const&, WebCore::AXTextStateChangeIntent const&, WebCore::VisibleSelection const&)+0x11a (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x29996fa)
    #14 0x60d9533d2 in WebCore::FrameSelection::notifyAccessibilityForSelectionChange(WebCore::AXTextStateChangeIntent const&)+0x202 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0xf903d2)
    #15 0x60ff8ff03 in WebCore::FrameSelection::updateAndRevealSelection(WebCore::AXTextStateChangeIntent const&)+0x223 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x35ccf03)
    #16 0x60ff9d02d in WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange()+0x7d (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x35da02d)
    #17 0x60ff9cf9c in WebCore::FrameSelection::updateAppearanceAfterLayout()+0x1c (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x35d9f9c)
    #18 0x610c1e799 in WebCore::FrameView::performPostLayoutTasks()+0x29 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x425b799)
    #19 0x610c628cd in WebCore::FrameViewLayoutContext::runAsynchronousTasks()+0x11d (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x429f8cd)
    #20 0x610c63a43 in WebCore::FrameViewLayoutContext::runOrScheduleAsynchronousTasks()+0x83 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x42a0a43)
    #21 0x610c631de in WebCore::FrameViewLayoutContext::layout()+0x79e (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x42a01de)
    #22 0x6118d6c8d in WebCore::RenderWidget::updateWidgetPosition()+0x17d (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x4f13c8d)
    #23 0x610c1b3a5 in WebCore::FrameView::updateWidgetPositions()+0x115 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x42583a5)
    #24 0x610c1e837 in WebCore::FrameView::performPostLayoutTasks()+0xc7 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x425b837)
    #25 0x610c628cd in WebCore::FrameViewLayoutContext::runAsynchronousTasks()+0x11d (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x429f8cd)
    #26 0x610c63a43 in WebCore::FrameViewLayoutContext::runOrScheduleAsynchronousTasks()+0x83 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x42a0a43)
    #27 0x610c631de in WebCore::FrameViewLayoutContext::layout()+0x79e (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x42a01de)
    #28 0x60fbf9072 in WebCore::Document::updateLayout()+0x212 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x3236072)
    #29 0x60fbfb422 in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks)+0x92 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x3238422)
    #30 0x611fc047a in WebCore::SVGTextContentElement::getComputedTextLength()+0xba (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x55fd47a)
    #31 0x60e1027f8 in WebCore::jsSVGTextContentElementPrototypeFunctionGetComputedTextLengthBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSSVGTextContentElement*, JSC::ThrowScope&)+0xb8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x173f7f8)
    #32 0x60e0510f3 in long long WebCore::IDLOperation<WebCore::JSSVGTextContentElement>::call<&(WebCore::jsSVGTextContentElementPrototypeFunctionGetComputedTextLengthBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSSVGTextContentElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)+0xf3 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x168e0f3)
    #33 0x60e050ff8 in WebCore::jsSVGTextContentElementPrototypeFunctionGetComputedTextLength(JSC::JSGlobalObject*, JSC::CallFrame*)+0x8 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/WebCore.framework/Versions/A/WebCore:x86_64+0x168dff8)
    #34 0x41021fa01177  (<unknown module>)
    #35 0x62916b6ba in llint_entry+0x170f3 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xb046ba)
    #36 0x62916b53d in llint_entry+0x16f76 (/private/tmp/tmpdoy6g092/working/20383157-37ab-42c0-b3c9-11276d07b83e/targets/mnt/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xb0453d)
    #37 0x629154418 in vmEntryToJavaScript

[Jack]

Should not have security implications since we are accessing the parent of renderView.

[zalan]

Please remove the security classification.

[Jack]

Created attachment 393913 [details]
Patch

[zalan]

Comment on attachment 393913 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Stop looking for sibling in AccessibilityRenderObject::nextSibling if the renderer of the AccessibilityObject is <RenderView>. 

This is where you should explain why it is ok to call this function on the RenderView. Also "stop looking" is a bit misleading since it's fine to look and not find any.

[Jack]

Created attachment 394043 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 394043 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

In this case, Antti already already reviewed the patch so you should fill this field with "Reviewed by Antti Koivisto"
If you hadn't already uploaded this patch, you could have done: webkit-patch land-safely.
That would have automatically carried Antti's r+ onto this patch and uploaded a new one for you.

[EWS]

Committed r258756: <https://trac.webkit.org/changeset/258756>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394043 [details].

[Darin Adler]

Comment on attachment 394043 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Itâs perfectly fine to call AccessibilityRenderObject::nextSibling on the RenderView (empty document) and since the RenderView has no sibling, letâs just early return with nullptr.

This comment still does not explain why the code is needed. A casual reading of the function seems to indicate it would work, and return nullptr, if it was a RenderView. I would have expected the fix to be making the code that fails on RenderView guard itself better and make fewer assumptions rather than putting a check at the very top of this function.

I don’t object to the code change, but I still find this to be a mysterious comment.

[chris fleizach]

should this fix be somewhere lower in the stack calling

isInlineWithContinuation(WebCore::RenderObject&) on a valid renderObject doesn't seem like it should ever crash

[Darin Adler]

(In reply to chris fleizach from comment #9)
> isInlineWithContinuation(WebCore::RenderObject&) on a valid renderObject
> doesn't seem like it should ever crash

Exactly.

[Jack]

Darin, Chris, thanks for the comments.

Yeah we had similar discussions about the fix. Putting a null check at isInlineWithContinuation(*m_renderer->parent()) works.

However, the function "postTextStateChangeNotification" seems to be looking for any sibling that has "unignored" accessibility. Since renderView would fail the sibling search, handling it earlier may help avoid potential issues. We thought about checking renderView in postTextStateChangeNotification before looking for any sibling.

However, it is indeed weird to only check for renderView but not other sibling-less cases.

Should I create a new patch if a localized fix is preferred?

[Jack]

Reopen to improve the patch.

[Darin Adler]

(In reply to Jack from comment #11)
> Since renderView would
> fail the sibling search, handling it earlier may help avoid potential
> issues.

I don’t know what "potential issues" you are talking about.

[zalan]

(In reply to chris fleizach from comment #9)
> should this fix be somewhere lower in the stack calling
> 
> isInlineWithContinuation(WebCore::RenderObject&) on a valid renderObject
> doesn't seem like it should ever crash
I don't think so. We know that the RenderView can never have a sibling. It is a condition that we can early return on.

[zalan]

(In reply to zalan from comment #14)
> (In reply to chris fleizach from comment #9)
> > should this fix be somewhere lower in the stack calling
> > 
> > isInlineWithContinuation(WebCore::RenderObject&) on a valid renderObject
> > doesn't seem like it should ever crash
> I don't think so. We know that the RenderView can never have a sibling. It
> is a condition that we can early return on.
(the "I don't think so" was a reply to "should this fix be somewhere lower in the stack calling")

[Darin Adler]

What was crashing without this check, and why was it crashing?

[zalan]

(In reply to Darin Adler from comment #16)
> What was crashing without this check, and why was it crashing?

"if (!nextSibling && isInlineWithContinuation(*m_renderer->parent()))"
nullptr deref. The logic further down in this function assumes that the current renderer always have a value parent (so it is attached to the render tree and we never call this function on the RenderView).

[zalan]

(In reply to zalan from comment #17)
> (In reply to Darin Adler from comment #16)
> > What was crashing without this check, and why was it crashing?
> 
> "if (!nextSibling && isInlineWithContinuation(*m_renderer->parent()))"
> nullptr deref. The logic further down in this function assumes that the
> current renderer always have a value parent (so it is attached to the render
> tree and we never call this function on the RenderView).
valid parent even.

[Jack]

Sorry I should rephrase it that we can prevent future issues if renderView is called in other functions (that eventually calls nextSibling).

(In reply to Darin Adler from comment #13)
> (In reply to Jack from comment #11)
> > Since renderView would
> > fail the sibling search, handling it earlier may help avoid potential
> > issues.
> 
> I don’t know what "potential issues" you are talking about.

[Jack]

Sorry I should rephrase it that we can prevent future issues if renderView is called in other functions (that eventually calls nextSibling).

(In reply to Darin Adler from comment #13)
> (In reply to Jack from comment #11)
> > Since renderView would
> > fail the sibling search, handling it earlier may help avoid potential
> > issues.
> 
> I don’t know what "potential issues" you are talking about.

[Jack]

Sorry, stale comment went out because weird collision. This is the new one:

Is it okay to patch the function like this:

    if (!m_renderer || !m_renderer->parent())
        return nullptr;

Is it safe to say that there is no sibling if parent is null?

[zalan]

(In reply to Jack from comment #21)
> Sorry, stale comment went out because weird collision. This is the new one:
> 
> Is it okay to patch the function like this:
> 
>     if (!m_renderer || !m_renderer->parent())
>         return nullptr;
This would be logically incorrect. It would certainly fix the null deref, but just because a renderer has no parent it does not necessarily mean it has no sibling either. The renderview (which is a very special renderer, it's the ICB) can never have a sibling and that's why it's okay to early return (and it happens to not have a parent either, but that's just implementation detail).

[Jack]

I see. Thanks. So there is no better way to check all the sibling-less cases.... hmm....

(In reply to zalan from comment #22)
> (In reply to Jack from comment #21)
> > Sorry, stale comment went out because weird collision. This is the new one:
> > 
> > Is it okay to patch the function like this:
> > 
> >     if (!m_renderer || !m_renderer->parent())
> >         return nullptr;
> This would be logically incorrect. It would certainly fix the null deref,
> but just because a renderer has no parent it does not necessarily mean it
> has no sibling either. The renderview (which is a very special renderer,
> it's the ICB) can never have a sibling and that's why it's okay to early
> return (and it happens to not have a parent either, but that's just
> implementation detail).

[Darin Adler]

We should not dereference the parent pointer without checking it for null. Please add the missing null check.

After adding that check for null, if you still want to have a special case for RenderView, that’s OK with me, although it doesn’t seem important or helpful.

[zalan]

(In reply to Darin Adler from comment #24)
> We should not dereference the parent pointer without checking it for null.
> Please add the missing null check.
> 
> After adding that check for null, if you still want to have a special case
> for RenderView, that’s OK with me, although it doesn’t seem important or
> helpful.
That's fine, but then the null check should be moved to where the deref happens. A seemingly random check for parent at the beginning of the function is incorrect (and drop the RenderView check)

[zalan]

(In reply to zalan from comment #25)
> (In reply to Darin Adler from comment #24)
> > We should not dereference the parent pointer without checking it for null.
> > Please add the missing null check.
> > 
> > After adding that check for null, if you still want to have a special case
> > for RenderView, that’s OK with me, although it doesn’t seem important or
> > helpful.
> That's fine, but then the null check should be moved to where the deref
> happens. A seemingly random check for parent at the beginning of the
> function is incorrect (and drop the RenderView check)
The reason why I thought checking against RenderView is the way to go was simply because how we can end up in the function with a nullptr parent:
1. m_renderer is the RenderView.
2. the m_renderer is detached from the tree.
and while the #1 is perfectly fine, #2 looks suspicious and probably indicates that the caller made a mistake (and should probably ASSERT)
but I can see why checking against nullptr is more robust and less error prone.

[Jack]

Thank you all very much for the discussion.

Browser is really complicated. Actually just a few lines down in this function, there are some potential null dereferences. I just don't know if any valid scenario exists that would trigger them.

        RenderElement* lastParent = endOfContinuations(*downcast<RenderBlock>(*m_renderer).lastChild())->parent();
//is it possible that lastChild() returns null?
        while (lastChildHasContinuation(*lastParent))
            lastParent = endOfContinuations(*lastParent->lastChild())->parent();
            //is it possible that lastChild() returns null?
            //is it possible that lastParent is null?


I will only patch for this test case to avoid confusion though.

[Jack]

Thanks Alan. I see. Yeah, there are quite some assumptions in the code.

I will add an ASSERTION for (m_renderer->parent() || is<RenderView>(*m_renderer)). This way if people hit null parent for invalid reason (#2), they can be warned.

(In reply to zalan from comment #26)> 1. m_renderer is the RenderView.
> 2. the m_renderer is detached from the tree.
> and while the #1 is perfectly fine, #2 looks suspicious and probably
> indicates that the caller made a mistake (and should probably ASSERT)
> but I can see why checking against nullptr is more robust and less error
> prone.

[zalan]

(In reply to Jack from comment #28)
> Thanks Alan. I see. Yeah, there are quite some assumptions in the code.
> 
> I will add an ASSERTION for (m_renderer->parent() ||
> is<RenderView>(*m_renderer)).
We usually don't do both (assert and check).

[Jack]

Oh, okay. I will just add the check.

(In reply to zalan from comment #29)
> (In reply to Jack from comment #28)
> > Thanks Alan. I see. Yeah, there are quite some assumptions in the code.
> > 
> > I will add an ASSERTION for (m_renderer->parent() ||
> > is<RenderView>(*m_renderer)).
> We usually don't do both (assert and check).

[Jack]

Created attachment 394156 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 394043 [details]
Patch

Let's not obsolete this patch since it got landed.

[Darin Adler]

(In reply to zalan from comment #25)
> (In reply to Darin Adler from comment #24)
> > We should not dereference the parent pointer without checking it for null.
> > Please add the missing null check.
> > 
> > After adding that check for null, if you still want to have a special case
> > for RenderView, that’s OK with me, although it doesn’t seem important or
> > helpful.
>
> That's fine, but then the null check should be moved to where the deref
> happens. A seemingly random check for parent at the beginning of the
> function is incorrect (and drop the RenderView check)

Yes, I wasn’t suggesting we add a check the top of the function.

[EWS]

Committed r258816: <https://trac.webkit.org/changeset/258816>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394156 [details].