Bug 265849 – AX: AccessibilityNodeObject::mouseButtonListener does not null-check Node::parentElement

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

[Tyler Wilcock]

This can cause a nullptr deref.

[Radar WebKit Bug Importer]

<rdar://problem/119167646>

[Tyler Wilcock]

rdar://116429648

[Tyler Wilcock]

Created attachment 468881 [details]
Patch

[EWS]

Committed 271562@main (85ef0cf14b23): <https://commits.webkit.org/271562@main>

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

[David Kilzer (:ddkilzer)]

Comment on attachment 468881 [details]
Patch

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

r=me, but I think we should use a `RefPtr` here if possible.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1325
> -    Node* node = this->node();
> +    WeakPtr node = this->node();
>      if (!node)
>          return nullptr;

Using a `WeakPtr node` here is a good first-order change to stop using a raw pointer, but I think we want to keep this `Node` alive for the duration of the method since we're using it to iterate through a list of other elements.

Can we use `RefPtr node` here instead?

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 468881 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468881&action=review
> 
> r=me, but I think we should use a `RefPtr` here if possible.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1325
> > -    Node* node = this->node();
> > +    WeakPtr node = this->node();
> >      if (!node)
> >          return nullptr;
> 
> Using a `WeakPtr node` here is a good first-order change to stop using a raw
> pointer, but I think we want to keep this `Node` alive for the duration of
> the method since we're using it to iterate through a list of other elements.
> 
> Can we use `RefPtr node` here instead?

Sorry, when I clicked on the review link, I forgot that no "cq" flag meant it had already been reviewed.  (I should have been looking for a "cq?" flag.)

This doesn't require a follow-up fix, but I think we eventually want to use a `RefPtr` here to keep the `node` alive for the duration of the method.