Prevent child frames from sending bad accessibility events.

Prevent child frames from sending bad accessibility events.

A recent change in M51 to have a separate accessibility tree for
each frame resulted in a subtle regression, where the
BrowserAccessibilityManager for a child frame would send
accessibility events before it was fully connected to its parent
frame. Because of how accessibility events work on Windows, when
an external accessibility client tried to respond to those events
the target node wouldn't be found when calling get_accChild to
retrieve it.

The user-visible manifestation was that NVDA spoke "unknown" a lot,
in response to those bogus events.

The fundamental idea behind the fixes is that a native accessibility
event shouldn't be fired unless the target node is a direct descendant
of the HWND for that tab. This logic needs to handle all of the cases
that arise when we have a separate BrowserAccessibilityManager for
each frame. The specific issues were:

* BrowserAccessibilityManager::GetRootManager was walking up to a parent
  frame without checking that the parent frame linked back to itself.
* MaybeCallNotifyWinEvent was checking its own delegate's HWND rather
  than the target node's delegate's HWND.
* RenderFrameHostImpl::AccessibilityGetAcceleratedWidget should only
  return a valid HWND for the main frame's current frame host.

BUG=600145
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fadccf76e38b76a0eb66f750ee668e89754596b7
Cr-Commit-Position: refs/heads/master@{#387681}

[dmazzoni, 2016-04-14 23:00:48]

Description was changed from

==========
Prevent child frames from sending bad accessibility events.

A recent change in M51 to have a separate accessibility tree for
each frame resulted in a subtle regression, where the
BrowserAccessibilityManager for a child frame would send
accessibility events before it was fully connected to its parent
frame. Because of how accessibility events work on Windows, when
an external accessibility client tried to respond to those events
the target node wouldn't be found when calling get_accChild to
retrieve it.

The user-visible manifestation was that NVDA spoke "unknown" a lot,
in response to those bogus events.

The fundamental idea behind the fixes is that a native accessibility
event shouldn't be fired unless the target node is a direct descendant
of the HWND for that tab. This logic needs to handle all of the cases
that arise when we have a separate BrowserAccessibilityManager for
each frame. The specific issues were:

* BrowserAccessibilityManager::GetRootManager was walking up to a parent
  frame without checking that the parent frame linked back to itself.
* MaybeCallNotifyWinEvent was checking its own delegate's HWND rather
  than the target node's delegate's HWND.
* RenderFrameHostImpl::AccessibilityGetAcceleratedWidget should only
  return a valid HWND for the main frame's current frame host.

BUG=600145
==========

to

==========
Prevent child frames from sending bad accessibility events.

A recent change in M51 to have a separate accessibility tree for
each frame resulted in a subtle regression, where the
BrowserAccessibilityManager for a child frame would send
accessibility events before it was fully connected to its parent
frame. Because of how accessibility events work on Windows, when
an external accessibility client tried to respond to those events
the target node wouldn't be found when calling get_accChild to
retrieve it.

The user-visible manifestation was that NVDA spoke "unknown" a lot,
in response to those bogus events.

The fundamental idea behind the fixes is that a native accessibility
event shouldn't be fired unless the target node is a direct descendant
of the HWND for that tab. This logic needs to handle all of the cases
that arise when we have a separate BrowserAccessibilityManager for
each frame. The specific issues were:

* BrowserAccessibilityManager::GetRootManager was walking up to a parent
  frame without checking that the parent frame linked back to itself.
* MaybeCallNotifyWinEvent was checking its own delegate's HWND rather
  than the target node's delegate's HWND.
* RenderFrameHostImpl::AccessibilityGetAcceleratedWidget should only
  return a valid HWND for the main frame's current frame host.

BUG=600145
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-04-14 23:01:24]

dmazzoni@chromium.org changed reviewers:
+ nektar@chromium.org

[dmazzoni, 2016-04-14 23:02:45]

dmazzoni@chromium.org changed reviewers:
+ jam@chromium.org

[dmazzoni, 2016-04-14 23:02:45]

+jam for content/browser/frame_host

[nektarios, 2016-04-15 14:44:49]

BrowserAccessibilityDelegate* delegate =
      node->manager()->GetDelegateFromRootManager();

Do we need to check if (node && node->instance_active()) first?

[nektarios, 2016-04-15 14:44:58]

lgtm

[jam, 2016-04-15 15:25:20]

On 2016/04/14 23:02:45, dmazzoni wrote:
> +jam for content/browser/frame_host

can you send this to someone working on site isolation? creis/nasko etc.
although this looks like a simple change, they're best suited to keep track of
these fixes and spot something i wouldn't

[dmazzoni, 2016-04-15 15:48:49]

dmazzoni@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org
- jam@chromium.org

[dmazzoni, 2016-04-15 15:48:49]

+dcheng (more familiar with accessibility code in content/browser/frame_host but
not an owner)
+creis (owner of content/)

[dcheng, 2016-04-15 17:02:09]

I forget: is the main frame inside an embedded WebContents also connected to the
native widget tree for accessibility?

[dmazzoni, 2016-04-15 17:37:47]

On 2016/04/15 17:02:09, dcheng wrote:
> I forget: is the main frame inside an embedded WebContents also connected to
the
> native widget tree for accessibility?

If its frame's RWHV is a RWHVChildFrame or RWHVGuest, then those already return
null
for AccessibilityGetAcceleratedWidget. So no they're not connected to the native
widget
tree, and those cases already behaved as intended.

The reason this fix is needed is because now even same-process RFHs have their
own
accessibility tree, but they share the same RWHV so they were all returning
something
from AccessibilityGetAcceleratedWidget and we were incorrectly posting events
from
iframes that weren't attached yet.

[dcheng, 2016-04-15 17:42:07]

lgtm

[dmazzoni, 2016-04-15 17:46:01]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-04-15 17:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892593003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892593003/1

[commit-bot: I haz the power, 2016-04-15 17:55:25]

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

[commit-bot: I haz the power, 2016-04-15 17:55:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Reis, 2016-04-15 19:50:20]

content/ LGTM.  Is it possible to follow up with a test (e.g., in case someone
later tries to "optimize" walking up the tree parent-by-parent by calling
GetRoot())?

[dmazzoni, 2016-04-15 19:52:25]

Yes, good idea. While end-to-end testing is quite
tricky there are definitely some unit tests we could
add to avoid regressing some of the pieces of this
fix.

[dmazzoni, 2016-04-15 19:52:31]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-04-15 19:52:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892593003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892593003/1

[commit-bot: I haz the power, 2016-04-15 19:57:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-15 19:59:37]

Description was changed from

==========
Prevent child frames from sending bad accessibility events.

A recent change in M51 to have a separate accessibility tree for
each frame resulted in a subtle regression, where the
BrowserAccessibilityManager for a child frame would send
accessibility events before it was fully connected to its parent
frame. Because of how accessibility events work on Windows, when
an external accessibility client tried to respond to those events
the target node wouldn't be found when calling get_accChild to
retrieve it.

The user-visible manifestation was that NVDA spoke "unknown" a lot,
in response to those bogus events.

The fundamental idea behind the fixes is that a native accessibility
event shouldn't be fired unless the target node is a direct descendant
of the HWND for that tab. This logic needs to handle all of the cases
that arise when we have a separate BrowserAccessibilityManager for
each frame. The specific issues were:

* BrowserAccessibilityManager::GetRootManager was walking up to a parent
  frame without checking that the parent frame linked back to itself.
* MaybeCallNotifyWinEvent was checking its own delegate's HWND rather
  than the target node's delegate's HWND.
* RenderFrameHostImpl::AccessibilityGetAcceleratedWidget should only
  return a valid HWND for the main frame's current frame host.

BUG=600145
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Prevent child frames from sending bad accessibility events.

A recent change in M51 to have a separate accessibility tree for
each frame resulted in a subtle regression, where the
BrowserAccessibilityManager for a child frame would send
accessibility events before it was fully connected to its parent
frame. Because of how accessibility events work on Windows, when
an external accessibility client tried to respond to those events
the target node wouldn't be found when calling get_accChild to
retrieve it.

The user-visible manifestation was that NVDA spoke "unknown" a lot,
in response to those bogus events.

The fundamental idea behind the fixes is that a native accessibility
event shouldn't be fired unless the target node is a direct descendant
of the HWND for that tab. This logic needs to handle all of the cases
that arise when we have a separate BrowserAccessibilityManager for
each frame. The specific issues were:

* BrowserAccessibilityManager::GetRootManager was walking up to a parent
  frame without checking that the parent frame linked back to itself.
* MaybeCallNotifyWinEvent was checking its own delegate's HWND rather
  than the target node's delegate's HWND.
* RenderFrameHostImpl::AccessibilityGetAcceleratedWidget should only
  return a valid HWND for the main frame's current frame host.

BUG=600145
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fadccf76e38b76a0eb66f750ee668e89754596b7
Cr-Commit-Position: refs/heads/master@{#387681}
==========

[commit-bot: I haz the power, 2016-04-15 19:59:38]

Patchset 1 (id:??) landed as
https://crrev.com/fadccf76e38b76a0eb66f750ee668e89754596b7
Cr-Commit-Position: refs/heads/master@{#387681}