Better handle null delegates by returning instead of CHECK()

Better handle null delegates by returning instead of CHECK()

BUG=741764
Review-Url: https://codereview.chromium.org/2976913002
Cr-Commit-Position: refs/heads/master@{#486221}
Committed: https://chromium.googlesource.com/chromium/src/+/2e19ce1cadce6b9a116f7795cb944a0c695b7735

[dougt, 2017-07-12 20:46:12]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-12 20:46:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-07-12 20:48:58]

dougt@chromium.org changed reviewers:
+ dtseng@chromium.org

[dougt, 2017-07-12 20:48:59]

dtseng, ptal.

[commit-bot: I haz the power, 2017-07-12 21:00:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-12 21:00:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chromium-reviews, 2017-07-12 21:11:58]

I thought we were checking for instance_active when making calls from native
objects whose lifetime might outlive their AXNode.  Why are we hitting this
bug?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-07-12 21:45:57]

Have you verified this is an access to a nullptr (delegate_) vs use after free
(delegate_ != nullptr but delegate_ points to deallocated heap)?

[David Tseng, 2017-07-12 21:47:14]

lgtm  I guess since it doesn't hurt, but I'm not convinced it's the underlying
issue from reading the stack trace for the crash.

[dougt, 2017-07-12 22:29:07]

On 2017/07/12 21:11:58, chromium-reviews wrote:
> I thought we were checking for instance_active when making calls from native
> objects whose lifetime might outlive their AXNode.  Why are we hitting this
> bug?


On the browser accessibility side, we do check owner() correctly. However, post
my changes we are handing out reference to the AXPlatformNodeWin.  When these
objects are used, they bypass the browser accessibility tests.  This is what we
want to have happen, eventually... It is my mistake for not realizing that this
would have happened as soon as landing the table code.

[dougt, 2017-07-12 22:30:49]

On 2017/07/12 21:45:57, David Tseng wrote:
> Have you verified this is an access to a nullptr (delegate_) vs use after free
> (delegate_ != nullptr but delegate_ points to deallocated heap)?

Good question, and yes. Destroy is called on some of the nodes then they are
used. This is a typical pattern and is handled correctly in the
BrowserAccessibilityComWin file.  This CL will provide similar protection
against destroyed nodes.

[dougt, 2017-07-12 22:38:25]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-12 22:38:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2017-07-12 22:48:45]

Ok, still lgtm. What I didn't see or check was that on destruction of an owner
of an AXPlatformNode*, you are always setting delegate_ to nullptr, but I trust
that's what always happens. I also haven't kept up with the latest expected
ownership/lifetime between BrowserAccessibility* and AXPlatformNode* fwiw.

https://codereview.chromium.org/2976913002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/2976913002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_base.cc:358: if (!delegate_)
Optional: This is such a common pattern I am tempted to suggest you define a
macro like
RETURN_IF_NULL(delegate_, nullptr)

[dougt, 2017-07-12 23:36:43]

>
https://codereview.chromium.org/2976913002/diff/1/ui/accessibility/platform/a...
> File ui/accessibility/platform/ax_platform_node_base.cc (right):
> 
>
https://codereview.chromium.org/2976913002/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/ax_platform_node_base.cc:358: if (!delegate_)
> Optional: This is such a common pattern I am tempted to suggest you define a
> macro like
> RETURN_IF_NULL(delegate_, nullptr)

This is a good idea and what we do in ax_platform_node_win.cc. There we use
COM_OBJECT_VALIDATE* and friends which test for delegate_. I'll do this in a
follow up if you don't mind too much.  I'd like to try to get this CL in to the
next canary.

[commit-bot: I haz the power, 2017-07-13 01:33:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 01:33:10]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-07-13 02:05:32]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-07-13 02:05:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 02:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499911532415190,
"parent_rev": "558367a792a97c67ae1594dac64148f4ba44bdc1", "commit_rev":
"2e19ce1cadce6b9a116f7795cb944a0c695b7735"}

[commit-bot: I haz the power, 2017-07-13 02:11:21]

Description was changed from

==========
Better handle null delegates by returning instead of CHECK()

BUG=741764
==========

to

==========
Better handle null delegates by returning instead of CHECK()

BUG=741764

Review-Url: https://codereview.chromium.org/2976913002
Cr-Commit-Position: refs/heads/master@{#486221}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e19ce1cadce6b9a116f7795cb94...
==========

[commit-bot: I haz the power, 2017-07-13 02:11:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2e19ce1cadce6b9a116f7795cb94...