Track all changed nodes during an update

Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify delegates of nodes that will actually be deleted and not just deleted then re-created as part of bookkeeping to enforce the "non-reparenting" invariant inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a client) does not destroy then re-create js objects; rather, ensure it re-uses them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[David Tseng, 2016-06-20 21:38:03]

Description was changed from

==========
Fix anotherautomation  out of sync case.
==========

to

==========
Fix anotherautomation  out of sync case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-20 21:43:06]

Description was changed from

==========
Fix anotherautomation  out of sync case.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix another automation/ax out of sync case

This is another issue with re-parenting.

Automation internal bindings incorrectly reported nodes deleted prematurely
while an update was still in process. In some cases such as in the Google Docs
menubar, nodes are reparented. Since we're more strict about detaching removed
nodes now, this showed up as a live node that had a backing native object, but
was seen as a detached js object (root-less).

With this fix, we no longer need to ignore ancestry chain announcements for
menuItem's. This also fixes the reading of the ChromeVox panel.

Leaving the checkbox and radio cases unchanged for now.

BUG=621665
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-20 21:43:06]

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

[David Tseng, 2016-06-20 21:44:39]

This is an interesting case where Docs appears to be doing something unexpected.
Note that VoiceOver on Mac has very similar problems with the menu and is unable
to announce index/total count likely because the underlying tree is so volitile.

[dmazzoni, 2016-06-20 22:47:23]

https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):

https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1101:
api::automation::TREE_CHANGE_TYPE_NODEREMOVED,
I think you still need to call this, otherwise we
can't support live regions that specifically want
notifications for removed nodes (aria-relevant="removals").

Perhaps ChromeVox's live_regions.js should handle the
"nodeRemoved" tree change but defer actually processing it,
and only handle it if the node was not reparented?

Note that automation needs to fire the tree change event now
because if we call it any later, the node won't exist anymore.

[David Tseng, 2016-06-22 17:03:41]

On 2016/06/20 22:47:23, dmazzoni wrote:
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):
> 
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> chrome/renderer/extensions/automation_internal_custom_bindings.cc:1101:
> api::automation::TREE_CHANGE_TYPE_NODEREMOVED,
> I think you still need to call this, otherwise we
> can't support live regions that specifically want
> notifications for removed nodes (aria-relevant="removals").
> 
> Perhaps ChromeVox's live_regions.js should handle the
> "nodeRemoved" tree change but defer actually processing it,
> and only handle it if the node was not reparented?
> 
> Note that automation needs to fire the tree change event now
> because if we call it any later, the node won't exist anymore.

This code (including the remove_node_ids_) is all a workaround for the
serializer's loose calculation of the lca (least common ancestor) which leads to
a node to clear far too high in the tree. I think we should just address that
since it's likely to bite for all platforms in unexpected ways.

[dmazzoni, 2016-06-22 17:06:27]

On 2016/06/22 17:03:41, David Tseng wrote:
> On 2016/06/20 22:47:23, dmazzoni wrote:
> >
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> > File chrome/renderer/extensions/automation_internal_custom_bindings.cc
(left):
> > 
> >
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> > chrome/renderer/extensions/automation_internal_custom_bindings.cc:1101:
> > api::automation::TREE_CHANGE_TYPE_NODEREMOVED,
> > I think you still need to call this, otherwise we
> > can't support live regions that specifically want
> > notifications for removed nodes (aria-relevant="removals").
> > 
> > Perhaps ChromeVox's live_regions.js should handle the
> > "nodeRemoved" tree change but defer actually processing it,
> > and only handle it if the node was not reparented?
> > 
> > Note that automation needs to fire the tree change event now
> > because if we call it any later, the node won't exist anymore.
> 
> This code (including the remove_node_ids_) is all a workaround for the
> serializer's loose calculation of the lca (least common ancestor) which leads
to
> a node to clear far too high in the tree. I think we should just address that
> since it's likely to bite for all platforms in unexpected ways.

Yeah, I'd be nice to pre-scan and determine nodes that will be deleted and
reparented in advance, and a single callback (similar to OnAtomicUpdateFinished)
with the list of all of those, before any tree changes actually happen.

[David Tseng, 2016-06-24 21:22:37]

On 2016/06/22 17:06:27, dmazzoni wrote:
> On 2016/06/22 17:03:41, David Tseng wrote:
> > On 2016/06/20 22:47:23, dmazzoni wrote:
> > >
> >
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> > > File chrome/renderer/extensions/automation_internal_custom_bindings.cc
> (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2080573003/diff/1/chrome/renderer/extensions/...
> > > chrome/renderer/extensions/automation_internal_custom_bindings.cc:1101:
> > > api::automation::TREE_CHANGE_TYPE_NODEREMOVED,
> > > I think you still need to call this, otherwise we
> > > can't support live regions that specifically want
> > > notifications for removed nodes (aria-relevant="removals").
> > > 
> > > Perhaps ChromeVox's live_regions.js should handle the
> > > "nodeRemoved" tree change but defer actually processing it,
> > > and only handle it if the node was not reparented?
> > > 
> > > Note that automation needs to fire the tree change event now
> > > because if we call it any later, the node won't exist anymore.
> > 
> > This code (including the remove_node_ids_) is all a workaround for the
> > serializer's loose calculation of the lca (least common ancestor) which
leads
> to
> > a node to clear far too high in the tree. I think we should just address
that
> > since it's likely to bite for all platforms in unexpected ways.
> 
> Yeah, I'd be nice to pre-scan and determine nodes that will be deleted and
> reparented in advance, and a single callback (similar to
OnAtomicUpdateFinished)
> with the list of all of those, before any tree changes actually happen.

Just FYI. This reparenting issue causes VoiceOver similar issues.

In inbox, within a thread, hitting n and p causes the message div to be
reparented.

In both ChromeVox Next and VoiceOver, trying to move to the next objet/item
causes us to hit a boundary because the accessibility object is detached.

[David Tseng, 2016-09-06 22:29:26]

Description was changed from

==========
Fix another automation/ax out of sync case

This is another issue with re-parenting.

Automation internal bindings incorrectly reported nodes deleted prematurely
while an update was still in process. In some cases such as in the Google Docs
menubar, nodes are reparented. Since we're more strict about detaching removed
nodes now, this showed up as a live node that had a backing native object, but
was seen as a detached js object (root-less).

With this fix, we no longer need to ignore ancestry chain announcements for
menuItem's. This also fixes the reading of the ChromeVox panel.

Leaving the checkbox and radio cases unchanged for now.

BUG=621665
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Track all changed nodes during an update


While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.
==========

[David Tseng, 2016-09-06 22:29:56]

New approach. PTAL.

[David Tseng, 2016-09-06 22:33:23]

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

[commit-bot: I haz the power, 2016-09-06 22:33:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2016-09-06 22:49:44]

This approach lgtm.

This class has pretty good unit test coverage now, let's
keep it that way. Please add a unit test to ax_tree_unittest.cc
or update any existing test that this breaks and it should be
good.

https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree.cc (right):

https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree.cc:264: if (delegate_ && (!update_state ||
I don't think we support |update_state| being nullptr anywhere.
It's only a pointer because it's modifiable and because its
lifetime is less than the lifetime of the AXTree object.
I'd just make it a DCHECK instead.

[commit-bot: I haz the power, 2016-09-06 22:52:19]

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

[commit-bot: I haz the power, 2016-09-06 22:52:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[David Tseng, 2016-09-07 00:10:09]

On Tue, Sep 6, 2016 at 3:49 PM, <dmazzoni@chromium.org> wrote:

> This approach lgtm.
>
> This class has pretty good unit test coverage now, let's
> keep it that way. Please add a unit test to ax_tree_unittest.cc
> or update any existing test that this breaks and it should be
> good.
>
>
>
> https://codereview.chromium.org/2080573003/diff/20001/ui/
> accessibility/ax_tree.cc
> File ui/accessibility/ax_tree.cc (right):
>
> https://codereview.chromium.org/2080573003/diff/20001/ui/
> accessibility/ax_tree.cc#newcode264
> ui/accessibility/ax_tree.cc:264: if (delegate_ && (!update_state ||
> I don't think we support |update_state| being nullptr anywhere.
> It's only a pointer because it's modifiable and because its
> lifetime is less than the lifetime of the AXTree object.
> I'd just make it a DCHECK instead.
>
> This function does get called in the dctor; crashes pretty badly without
it so keeping this around.

> https://codereview.chromium.org/2080573003/
>

-- 
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.

[dmazzoni, 2016-09-07 05:00:09]

Ah, forgot about the dtor, that makes sense.

On Tue, Sep 6, 2016 at 5:10 PM David Tseng <dtseng@chromium.org> wrote:

> On Tue, Sep 6, 2016 at 3:49 PM, <dmazzoni@chromium.org> wrote:
>
> This approach lgtm.
>
> This class has pretty good unit test coverage now, let's
> keep it that way. Please add a unit test to ax_tree_unittest.cc
> or update any existing test that this breaks and it should be
> good.
>
>
>
>
>
https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
> File ui/accessibility/ax_tree.cc (right):
>
>
>
https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
> ui/accessibility/ax_tree.cc:264: if (delegate_ && (!update_state ||
> I don't think we support |update_state| being nullptr anywhere.
> It's only a pointer because it's modifiable and because its
> lifetime is less than the lifetime of the AXTree object.
> I'd just make it a DCHECK instead.
>
> This function does get called in the dctor; crashes pretty badly without
> it so keeping this around.
>
> https://codereview.chromium.org/2080573003/
>
>
> --
> 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.

[David Tseng, 2016-09-07 16:37:01]

Added test and also fixed the reparented/created OnNodeCreated case to not
notify delegates on a removed/created reparented node.

https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree.cc (right):

https://codereview.chromium.org/2080573003/diff/20001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree.cc:264: if (delegate_ && (!update_state ||
On 2016/09/06 22:49:44, dmazzoni wrote:
> I don't think we support |update_state| being nullptr anywhere.
> It's only a pointer because it's modifiable and because its
> lifetime is less than the lifetime of the AXTree object.
> I'd just make it a DCHECK instead.

Alright, done, DestroySubtree only gets called from Unserialize, so this appears
to be right.


Btw, are you ok with not notifying the delegate of a subtree deletion when the
AXTree is destroyed? I think that might be the last (hopefully) remaining corner
case.

[dmazzoni, 2016-09-07 16:54:09]

still lgtm, thanks for the test

On 2016/09/07 at 16:37:01, dtseng wrote:
> Btw, are you ok with not notifying the delegate of a subtree deletion when the
AXTree is destroyed? I think that might be the last (hopefully) remaining corner
case.

Yes, I don't think we need it.

[David Tseng, 2016-09-07 17:58:45]

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

[commit-bot: I haz the power, 2016-09-07 17:59:22]

Dry run: 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, 2016-09-07 19:50:24]

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

[commit-bot: I haz the power, 2016-09-07 19:50:25]

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

[David Tseng, 2016-09-07 21:33:17]

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

[commit-bot: I haz the power, 2016-09-07 21:34:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2016-09-07 21:49:33]

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

[commit-bot: I haz the power, 2016-09-07 21:50:40]

Dry run: 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, 2016-09-07 22:45:34]

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

[commit-bot: I haz the power, 2016-09-07 22:45:35]

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

[David Tseng, 2016-09-07 22:48:05]

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

[commit-bot: I haz the power, 2016-09-07 22:48:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2016-09-07 22:57:59]

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

[commit-bot: I haz the power, 2016-09-07 22:58:39]

Dry run: 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, 2016-09-07 23:21:08]

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

[commit-bot: I haz the power, 2016-09-07 23:21:09]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[David Tseng, 2016-09-08 00:29:55]

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

[commit-bot: I haz the power, 2016-09-08 00:30:11]

Dry run: 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, 2016-09-08 02:25:57]

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

[commit-bot: I haz the power, 2016-09-08 02:25:58]

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

[David Tseng, 2016-09-08 05:13:18]

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

[commit-bot: I haz the power, 2016-09-08 05:13:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2016-09-08 05:25:14]

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

[David Tseng, 2016-09-08 05:25:29]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-09-08 05:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2080573003/#ps160001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-09-08 05:25:31]

Dry run: 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, 2016-09-08 06:36:16]

Description was changed from

==========
Track all changed nodes during an update


While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.
==========

to

==========
Track all changed nodes during an update


While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.
==========

[commit-bot: I haz the power, 2016-09-08 06:36:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-08 06:38:34]

Description was changed from

==========
Track all changed nodes during an update


While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.
==========

to

==========
Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
==========

[commit-bot: I haz the power, 2016-09-08 06:38:36]

Patchset 9 (id:??) landed as
https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}

[David Tseng, 2016-09-09 18:04:10]

Description was changed from

==========
Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
==========

to

==========
Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[David Tseng, 2016-09-09 18:04:11]

Description was changed from

==========
Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing
any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify
delegates of nodes that will actually be deleted and not just deleted then
re-created as part of bookkeeping to enforce the "non-reparenting" invariant
inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a
client) does not destroy then re-create js objects; rather, ensure it re-uses
them for reparented trees.

Committed: https://crrev.com/5a7b3d983322a039f87553ae936b37984f81efcb
Cr-Commit-Position: refs/heads/master@{#417207}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========