Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929673002:
Add null check when reporting changed nodes to an AX tree. (Closed)
|
Created:
3 years, 6 months ago by dmazzoni Modified:
3 years, 5 months ago Reviewers:
David Tseng CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd null check when reporting changed nodes to an AX tree.
See bug for repro; this happens when we get an event fired on a window
that's not part of the window hierarchy. The rest of the code ignores it
fine but the tree change callbacks get called on a null node, which
causes problems.
BUG=727842
Review-Url: https://codereview.chromium.org/2929673002
Cr-Commit-Position: refs/heads/master@{#482381}
Committed: https://chromium.googlesource.com/chromium/src/+/b232d53c93cfa66c1e3647280cfdd694f52d29f0
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix underlying issue too #
Total comments: 8
Patch Set 3 : Fix logic error #Patch Set 4 : Simplest fix #
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + dtseng@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Seems harmless, but would it be clearer to fix this on the source end? IIRC, we parent all top level windows to the desktop node. https://codereview.chromium.org/2929673002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2929673002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:223: if (!node) At this point, I would have expected there to be pending nodes above? Why wasn't this caught by the deserialization below?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, addressed the true underlying cause now. The real issue here is that we were firing events on automation objects that were either invisible or somewhere in an invisible subtree. Those are usually harmless but there's a corner case where if you serialize an invisible node and then its visible parent, unserialization triggers this null check. The proper underlying fix, then, is to not even try to fire events on automation objects that aren't fully connected all the way up to the root desktop node. https://codereview.chromium.org/2929673002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2929673002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:223: if (!node) On 2017/06/07 23:24:24, David Tseng wrote: > At this point, I would have expected there to be pending nodes above? Why wasn't > this caught by the deserialization below? Imagine that B's parent is A, but B is invisible so A does not list B as a child. We were unserializing B and then A, in that order. B unserializes fine. Then we get to A, and A does not list B as a child anymore, so B gets deleted. Then later during postprocessing we search for B but it's no longer in the tree. It's a weird corner case but I don't think the unserialization logic was wrong other than this missing null check.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Friendly ping, this is a crash fix that needs to be merged
https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right): https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:153: // of ancestors all the way the root desktop node. The most common nit: to the https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:163: if (!parent) I can see why this happens, but I think the detached view should never have called NotifyAccessibilityEvent in the first place. Do we understand why the view triggers the event even if it is detached? Is this your location change event? Can you put a TODO at least with a bug to investigate? Also, was this crash a regression? Can we just revert the original change?
https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right): https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:155: // invisible subtree; the object itself thinks it's visible, but one One more note: if this is true, then I think View::NotifyAccessibilityEvent should perform the change. I see we currently treat native accessibility slightly differently. We check GetWidget() is non-null. So, add something like if (ViewsDelegate::GetInstance() && GetWidget() && GetWidget()->IsVisible()) ViewsDelegate::GetInstance()->NotifyAccessibilityEvent... and don't do this workaround here. https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:169: if (children[i] == walker) { Also, this doesn't make sense to me. Did you mean to check that walker is a child of itself? Or did you want to check the child's parent is walker? In either case, I'd rather fix the underlying problem in either views or windows where the bad parent child relationship occurs.
https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right): https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:153: // of ancestors all the way the root desktop node. The most common On 2017/06/21 15:44:46, David Tseng wrote: > nit: to the Done. https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:155: // invisible subtree; the object itself thinks it's visible, but one On 2017/06/21 16:09:50, David Tseng wrote: > One more note: if this is true, then I think View::NotifyAccessibilityEvent > should perform the change. I see we currently treat native accessibility > slightly differently. We check GetWidget() is non-null. > > So, add something like > if (ViewsDelegate::GetInstance() && GetWidget() && GetWidget()->IsVisible()) > ViewsDelegate::GetInstance()->NotifyAccessibilityEvent... > > and don't do this workaround here. I don't think that's robust. The problem is that both AXViewObjWrapper and AXWindowObjWrapper both check visibility in GetChildren(). Because of the window issue, we can't do the check in views - you could have a view that's visible, in a widget that's visible, in a window that's visible, but its *parent* window is not visible. I'm also concerned about cases like a window that's closed, but some events fire on its views after it's been detached from the window hierarchy but before it's been destroyed. https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:163: if (!parent) On 2017/06/21 15:44:46, David Tseng wrote: > I can see why this happens, but I think the detached view should never have > called NotifyAccessibilityEvent in the first place. Do we understand why the > view triggers the event even if it is detached? Is this your location change > event? > > Can you put a TODO at least with a bug to investigate? > > Also, was this crash a regression? Can we just revert the original change? Yes, this was the location change, but I think it could have happened otherwise, just much less likely. https://codereview.chromium.org/2929673002/diff/20001/chrome/browser/ui/aura/... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:169: if (children[i] == walker) { On 2017/06/21 16:09:50, David Tseng wrote: > Also, this doesn't make sense to me. Did you mean to check that walker is a > child of itself? Or did you want to check the child's parent is walker? You're right. The code above should have called parent->GetChildren(), not walker->GetChildren(). This patch was suppressing almost all events, that wouldn't have been good. Fixed now and it suppresses the events leading to a crash but not any others. > In either case, I'd rather fix the underlying problem in either views or windows > where the bad parent child relationship occurs. So we can't fix it in Views because Views can't check whether an aura::Window is visible. I think we'd have to fix it in HandleEvent or ChromeViewsDelegate unless we refactor things a lot.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Stepping back. Your cl description says: See bug for repro; this happens when we get an event fired on a window that's not part of the window hierarchy. The rest of the code ignores it fine but the tree change callbacks get called on a null node, which Is it sufficient for this change, which I assume you want to merge, to just not fire a location change on a window that's not part of the window hierarchy? I agree we should never have a a parent who's children->getParent isn't the parent itself; however, I still don't see what it has to do with the bug. I also don't think the validity check is right; we should correct the actual tree methods in AX*ObjWrapper classes to not form those incorrect relationships in the first place.
OK, I think you're right - let me fix this specific bug by not firing an event on an invisible window, then we can revisit this and see if there's a better way to implement the check at this level for other corner cases.
Great. Also, note that AXWindowObjWrapper::GetChildren explicitly ignores windows that are !IsVisible(). This could very well cause the tree parent child assymetry you're seeing if a sub window fires an event while it is invisible and make it appear as though it's not part of the window hierarchy. On Fri, Jun 23, 2017 at 2:47 PM, <dmazzoni@chromium.org> wrote: > OK, I think you're right - let me fix this specific bug by not > firing an event on an invisible window, then we can revisit this > and see if there's a better way to implement the check at this > level for other corner cases. > > > https://codereview.chromium.org/2929673002/ > > -- > 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. > -- 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.
OK, the minimal fix turned out to be in AXWindowObjWrapper. I'll do a change to Views in a follow-up but that wasn't necessary for now.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498508675857570,
"parent_rev": "30f9a63797e1403c229b6e54a773339593f9c017", "commit_rev":
"5ac95091b4d42a78dcb4c6f039e5f97411340326"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498508675857570,
"parent_rev": "a26c270f0ade55141141f4c23ab9e2c93de342c2", "commit_rev":
"b232d53c93cfa66c1e3647280cfdd694f52d29f0"}
Message was sent while issue was closed.
Description was changed from ========== Add null check when reporting changed nodes to an AX tree. See bug for repro; this happens when we get an event fired on a window that's not part of the window hierarchy. The rest of the code ignores it fine but the tree change callbacks get called on a null node, which causes problems. BUG=727842 ========== to ========== Add null check when reporting changed nodes to an AX tree. See bug for repro; this happens when we get an event fired on a window that's not part of the window hierarchy. The rest of the code ignores it fine but the tree change callbacks get called on a null node, which causes problems. BUG=727842 Review-Url: https://codereview.chromium.org/2929673002 Cr-Commit-Position: refs/heads/master@{#482381} Committed: https://chromium.googlesource.com/chromium/src/+/b232d53c93cfa66c1e3647280cfd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b232d53c93cfa66c1e3647280cfd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
