Check whether a text node may be about to be redistributed in ShouldUpdateLayoutByReattaching()

Check whether a text node may be about to be redistributed in ShouldUpdateLayoutByReattaching(). Prevents use-after-free when SetTextWithOffset is called with dirty distribution.

BUG=726716
Review-Url: https://codereview.chromium.org/2913133002
Cr-Commit-Position: refs/heads/master@{#476242}
Committed: https://chromium.googlesource.com/chromium/src/+/6281b70b9a36a64590f99c799ee55264da6cb52d

[aboxhall, 2017-05-31 05:56:50]

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

[aboxhall, 2017-05-31 05:56:50]

What do you think of this fix?

[aboxhall, 2017-05-31 05:56:51]

What do you think of this fix?

[dmazzoni, 2017-05-31 05:57:07]

Could you add another sentence to the change description
explaining why it needs to be last? How did calling text_autosizer->Record()
break anything?

[dmazzoni, 2017-05-31 06:04:19]

lgtm based on offline discussion, please explain a bit more
in the change description before submitting

[aboxhall, 2017-05-31 06:25:14]

Description was changed from

==========
Call AXObjectCache::TextChanged() last when handling SetText

BUG=726716
==========

to

==========
Call AXObjectCache::TextChanged() last when handling SetText.

Since TextChanged() may trigger UpdateDistribution, calling GetDocument()
immediately after may end up with a stale node reference.

BUG=726716
==========

[aboxhall, 2017-05-31 06:37:21]

aboxhall@chromium.org changed reviewers:
+ kojii@chromium.org

[aboxhall, 2017-05-31 06:37:22]

kojii@ - would you mind taking a look at this? I will CC you on the bug as well.

[kojii, 2017-05-31 14:25:55]

kojii@chromium.org changed reviewers:
+ eae@chromium.org, hayato@chromium.org

[kojii, 2017-05-31 14:25:55]

Hi Alice, thank you for working on this very promptly.

Let me cc eae@ and hayato@, you hit an unfotunate case that I'd like them to be
aware of. Please chime in if you have any.

I don't think we want to go with the assumption that calling
LayoutText::SetText() may delete itself, since we're likely to see other tests
to hit this condition.

In general, we should not modify layout tree during layout. crbug.com/370457 is
about not to modify layout tree outside of the style recalc, and
crbug.com/590369 is about hoisting updateStyleAndLayout() so that we update
layout tree as early as possible before starting work. Since
updateDistribution() can modify layout tree, I think we should do the similar
efforts as updateStyleAndLayout().

Here are a couple of my suggestions.

1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
objects dirty, and run the rest of work in the next lifecycle opporunity?

2. I suppose #1 is rather a long term work even if possible. Another suggestion
is to change  ShouldUpdateLayoutByReattaching() to return true when AX needs to
call UpdateDistribution(). Basically, SetText() should be called only during
style recalc/tree building, but CharacterData::appendData() calling SetText() is
much faster. This function determines whether it's safe to run an optimized code
path, and this case isn't safe. I'm not sure if the function can catch the
condition when AX needs to UpdateDistribution(), can you see if it's possible?

3. Hayato and I were discussing to make updateDistribution as a lifecycle, or
add DCHECK to ensure it's not called inproperly. Maybe we should reconsider
doing this?

#3 should probably fall onto DOM team. I'll file a separate bug.

#1 is great if you, Alice, or someone in AX can consider in long term. I fixed
use-after-free a couple of times before when AXObjectCacheImpl::TextChanged()
calls updateStyleAndLayout(), but it looks to me that this work is open-ended as
we improve AX. I can only imagine this is a big work, but I hope you'll get more
stable code by running in proper lifecycle.

I hope you can find the necessary condition for #2, but if it's too hard or
takes too long, I'm ok to land this and work on it later. Please let me know.

[kojii, 2017-05-31 14:34:35]

On 2017/05/31 at 14:25:55, kojii wrote:
> #3 should probably fall onto DOM team. I'll file a separate bug.

Filed crbug.com/728151

[dmazzoni, 2017-05-31 15:33:04]

On Wed, May 31, 2017 at 7:25 AM <kojii@chromium.org> wrote:

> 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
> objects dirty, and run the rest of work in the next lifecycle opporunity?
>

In parallel I'm working on some refactoring that will replace all
accessibility events with just marking objects dirty, but that's a few
weeks away before it will be done.

I think we need a simpler fix for right now that doesn't break any existing
tests.

Alice, what happens if we don't call UpdateDistribution inside
computeAccessibilityIsIgnored? Can we just punt if UpdateDistribution needs
to be called and hope we catch the tree change later?

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

[dmazzoni, 2017-05-31 15:33:05]

On Wed, May 31, 2017 at 7:25 AM <kojii@chromium.org> wrote:

> 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
> objects dirty, and run the rest of work in the next lifecycle opporunity?
>

In parallel I'm working on some refactoring that will replace all
accessibility events with just marking objects dirty, but that's a few
weeks away before it will be done.

I think we need a simpler fix for right now that doesn't break any existing
tests.

Alice, what happens if we don't call UpdateDistribution inside
computeAccessibilityIsIgnored? Can we just punt if UpdateDistribution needs
to be called and hope we catch the tree change later?

-- 
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, 2017-05-31 15:37:03]

The bug where I'm working on that refactoring is http://crbug.com/699438 -
i'm starting by improving test coverage for accessibility events, then
rewriting the event generating code outside of Blink, then as a last step
I'll be able to remove all of the event sending code and replace it with a
simple markDirty.


On Wed, May 31, 2017 at 8:32 AM Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Wed, May 31, 2017 at 7:25 AM <kojii@chromium.org> wrote:
>
>> 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
>> objects dirty, and run the rest of work in the next lifecycle opporunity?
>>
>
> In parallel I'm working on some refactoring that will replace all
> accessibility events with just marking objects dirty, but that's a few
> weeks away before it will be done.
>
> I think we need a simpler fix for right now that doesn't break any
> existing tests.
>
> Alice, what happens if we don't call UpdateDistribution inside
> computeAccessibilityIsIgnored? Can we just punt if UpdateDistribution needs
> to be called and hope we catch the tree change later?
>
>

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

[dmazzoni, 2017-05-31 15:37:04]

The bug where I'm working on that refactoring is http://crbug.com/699438 -
i'm starting by improving test coverage for accessibility events, then
rewriting the event generating code outside of Blink, then as a last step
I'll be able to remove all of the event sending code and replace it with a
simple markDirty.


On Wed, May 31, 2017 at 8:32 AM Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Wed, May 31, 2017 at 7:25 AM <kojii@chromium.org> wrote:
>
>> 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
>> objects dirty, and run the rest of work in the next lifecycle opporunity?
>>
>
> In parallel I'm working on some refactoring that will replace all
> accessibility events with just marking objects dirty, but that's a few
> weeks away before it will be done.
>
> I think we need a simpler fix for right now that doesn't break any
> existing tests.
>
> Alice, what happens if we don't call UpdateDistribution inside
> computeAccessibilityIsIgnored? Can we just punt if UpdateDistribution needs
> to be called and hope we catch the tree change later?
>
>

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

[aboxhall, 2017-05-31 22:29:13]

On 2017/05/31 15:33:04, dmazzoni wrote:
> On Wed, May 31, 2017 at 7:25 AM <mailto:kojii@chromium.org> wrote:
> 
> > 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
> > objects dirty, and run the rest of work in the next lifecycle opporunity?
> >
> 
> In parallel I'm working on some refactoring that will replace all
> accessibility events with just marking objects dirty, but that's a few
> weeks away before it will be done.
> 
> I think we need a simpler fix for right now that doesn't break any existing
> tests.
> 
> Alice, what happens if we don't call UpdateDistribution inside
> computeAccessibilityIsIgnored? Can we just punt if UpdateDistribution needs
> to be called and hope we catch the tree change later?

No, I added that call because things were crashing in other places without it.

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

[kojii, 2017-06-01 02:09:40]

On 2017/05/31 at 15:33:05, dmazzoni wrote:
> On Wed, May 31, 2017 at 7:25 AM <kojii@chromium.org> wrote:
> 
> > 1. Could we make AXObjectCacheImpl::TextChanged() lighter, such as marking
> > objects dirty, and run the rest of work in the next lifecycle opporunity?
> >
> 
> In parallel I'm working on some refactoring that will replace all
> accessibility events with just marking objects dirty, but that's a few
> weeks away before it will be done.

Superb, I wasn't aware you're already working on it! And I was expecting months
of work, this is super exciting.

Thank you for working on this.

> I think we need a simpler fix for right now that doesn't break any existing
> tests.

Agreed.

On 2017/05/31 at 22:29:13, aboxhall wrote:
> 
> No, I added that call because things were crashing in other places without it.

I didn't debug this yet, but do you think you can try #2 then? I mean, catch
this condition in ShouldUpdateLayoutByReattaching()? If it returns true, we'll
LazyReattachIfAttached() instead of SetText().

[kojii, 2017-06-01 02:25:34]

On 2017/06/01 at 02:09:40, kojii wrote:
> On 2017/05/31 at 22:29:13, aboxhall wrote:
> > 
> > No, I added that call because things were crashing in other places without
it.
> 
> I didn't debug this yet, but do you think you can try #2 then? I mean, catch
this condition in ShouldUpdateLayoutByReattaching()? If it returns true, we'll
LazyReattachIfAttached() instead of SetText().

Like:

if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache()) {
  if (cache->MayNeedUpdateDistribution(text_layout_object))
    return true;
}

AXObjectCacheImpl::MayNeedUpdateDistribution(layout_object) {
  return !Get(layout_object);
}

By a quick look, it doesn't go to UpdateDistribution() if the object exists,
right?

It can be improved further if you'd like, to make it faster, but I hope
something like this can fix.

[aboxhall, 2017-06-01 02:57:04]

On 2017/06/01 02:25:34, kojii wrote:
> On 2017/06/01 at 02:09:40, kojii wrote:
> > On 2017/05/31 at 22:29:13, aboxhall wrote:
> > > 
> > > No, I added that call because things were crashing in other places without
> it.
> > 
> > I didn't debug this yet, but do you think you can try #2 then? I mean, catch
> this condition in ShouldUpdateLayoutByReattaching()? If it returns true, we'll
> LazyReattachIfAttached() instead of SetText().
> 
> Like:
> 
> if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache()) {
>   if (cache->MayNeedUpdateDistribution(text_layout_object))
>     return true;
> }
> 
> AXObjectCacheImpl::MayNeedUpdateDistribution(layout_object) {
>   return !Get(layout_object);
> }

Thanks for the further explanation! Exploring this now.

> By a quick look, it doesn't go to UpdateDistribution() if the object exists,
> right?

I believe that's right, yes.

> It can be improved further if you'd like, to make it faster, but I hope
> something like this can fix.

[aboxhall, 2017-06-01 06:19:45]

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

[commit-bot: I haz the power, 2017-06-01 06:19:59]

Dry run: CQ is trying da patch.

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

[aboxhall, 2017-06-01 06:20:00]

On 2017/06/01 02:57:04, aboxhall wrote:
> On 2017/06/01 02:25:34, kojii wrote:
> > On 2017/06/01 at 02:09:40, kojii wrote:
> > > On 2017/05/31 at 22:29:13, aboxhall wrote:
> > > > 
> > > > No, I added that call because things were crashing in other places
without
> > it.
> > > 
> > > I didn't debug this yet, but do you think you can try #2 then? I mean,
catch
> > this condition in ShouldUpdateLayoutByReattaching()? If it returns true,
we'll
> > LazyReattachIfAttached() instead of SetText().
> > 
> > Like:
> > 
> > if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache()) {
> >   if (cache->MayNeedUpdateDistribution(text_layout_object))
> >     return true;
> > }
> > 
> > AXObjectCacheImpl::MayNeedUpdateDistribution(layout_object) {
> >   return !Get(layout_object);
> > }
> 
> Thanks for the further explanation! Exploring this now.

Ok, made a different fix. PTAL.

> 
> > By a quick look, it doesn't go to UpdateDistribution() if the object exists,
> > right?
> 
> I believe that's right, yes.
> 
> > It can be improved further if you'd like, to make it faster, but I hope
> > something like this can fix.

[kojii, 2017-06-01 07:27:32]

lgtm w/nit.

It checks only direct parent, so there maybe cases where the text is a
descendants of the shadow. ContainingShadowRoot() can give you if you're
somewhere in a shadow tree, and it doesn't traverse so should be as fast as
ParentElementShadow(). If you try this and figure out it doesn't fix, the
current patch is fine.

Also, could you please add a comment that we need this because there's a call
path to UpdateDistribution() in LayoutText::SetText(), crbug.com/699438 is
trying to eliminate the call path so that this isn't needed when it's done.

Thank you again for working on this.

[kojii, 2017-06-01 07:32:29]

After seeing your message offline, I understand it's intentional to check parent
only, so disregard the first part.

Having a comment is helpful to remember this after a while, great if you could
add one.

[commit-bot: I haz the power, 2017-06-01 08:19:00]

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

[commit-bot: I haz the power, 2017-06-01 08:19:01]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-06-01 08:29:06]

Description was changed from

==========
Call AXObjectCache::TextChanged() last when handling SetText.

Since TextChanged() may trigger UpdateDistribution, calling GetDocument()
immediately after may end up with a stale node reference.

BUG=726716
==========

to

==========
Check whether a text node may be about to be redistributed in
ShouldUpdateLayoutByReattaching(). Prevents use-after-free in when
SetTextWithOffset is called with dirty distribution.

BUG=726716
==========

[aboxhall, 2017-06-01 08:29:16]

Description was changed from

==========
Check whether a text node may be about to be redistributed in
ShouldUpdateLayoutByReattaching(). Prevents use-after-free in when
SetTextWithOffset is called with dirty distribution.

BUG=726716
==========

to

==========
Check whether a text node may be about to be redistributed in
ShouldUpdateLayoutByReattaching(). Prevents use-after-free when
SetTextWithOffset is called with dirty distribution.

BUG=726716
==========

[aboxhall, 2017-06-01 08:29:25]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-06-01 08:29:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2913133002/#ps40001
(title: "Add comment")

[commit-bot: I haz the power, 2017-06-01 08:29:34]

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-06-01 10:09:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496305765681580,
"parent_rev": "1564e791a8a6678d9c550bf4acb78500a4c19d05", "commit_rev":
"6281b70b9a36a64590f99c799ee55264da6cb52d"}

[commit-bot: I haz the power, 2017-06-01 10:10:00]

Description was changed from

==========
Check whether a text node may be about to be redistributed in
ShouldUpdateLayoutByReattaching(). Prevents use-after-free when
SetTextWithOffset is called with dirty distribution.

BUG=726716
==========

to

==========
Check whether a text node may be about to be redistributed in
ShouldUpdateLayoutByReattaching(). Prevents use-after-free when
SetTextWithOffset is called with dirty distribution.

BUG=726716

Review-Url: https://codereview.chromium.org/2913133002
Cr-Commit-Position: refs/heads/master@{#476242}
Committed:
https://chromium.googlesource.com/chromium/src/+/6281b70b9a36a64590f99c799ee5...
==========

[commit-bot: I haz the power, 2017-06-01 10:10:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6281b70b9a36a64590f99c799ee5...

[yosin_UTC9, 2017-06-02 07:47:42]

yosin@chromium.org changed reviewers:
+ yosin@chromium.org

[yosin_UTC9, 2017-06-02 07:47:43]

https://codereview.chromium.org/2913133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/2913133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:451: if
(text_node.ParentElementShadow() &&
What is happend when Text is descendant of shadow host? 

e.g.
<div id="shadowHost"><span>My parent is not shadow host</span></div>

[hayato, 2017-06-02 08:00:29]

> After seeing your message offline, I understand it's intentional to check
parent only, so disregard the first part.

I appreciate if you could share us what is the intention behind? It looks wrong
to me.

[kojii, 2017-06-02 16:18:17]

On 2017/06/02 at 08:00:29, hayato wrote:
> > After seeing your message offline, I understand it's intentional to check
parent only, so disregard the first part.
> 
> I appreciate if you could share us what is the intention behind? It looks
wrong to me.

First, please see #13 and #14. dmazzoni@ is working on making AX more lifecycle
friendly, hopefully in a few weeks, so this is a fix until then.

Second, most of cases do not hit this condition, since there are many other
checks until the code reaches to UpdateDistribution(). So far the only case she
could find is when a text node is distributed into shadow dom. Maybe we could
also check if the parent is insertion point, if there were way to do it (I don't
know atm), but we can add it if such cases were found before dmazzoni@ finishes
his work.

I didn't measure by myself but this function is known to be a very special,
intentional rule breaker because of a significant performance benefit. Disabling
in all shadow trees without understanding a problem exists scares me in other
ways.

[aboxhall, 2017-06-07 00:36:32]

On 2017/06/02 16:18:17, kojii wrote:
> On 2017/06/02 at 08:00:29, hayato wrote:
> > > After seeing your message offline, I understand it's intentional to check
> parent only, so disregard the first part.
> > 
> > I appreciate if you could share us what is the intention behind? It looks
> wrong to me.
> 
> First, please see #13 and #14. dmazzoni@ is working on making AX more
lifecycle
> friendly, hopefully in a few weeks, so this is a fix until then.
> 
> Second, most of cases do not hit this condition, since there are many other
> checks until the code reaches to UpdateDistribution(). So far the only case
she
> could find is when a text node is distributed into shadow dom. Maybe we could
> also check if the parent is insertion point, if there were way to do it (I
don't
> know atm), but we can add it if such cases were found before dmazzoni@
finishes
> his work.
> 
> I didn't measure by myself but this function is known to be a very special,
> intentional rule breaker because of a significant performance benefit.
Disabling
> in all shadow trees without understanding a problem exists scares me in other
> ways.

Yes, I'm hoping this will only need to be a temporary fix.

In terms of @yosin's question, the object was being deleted in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
so I think this only affects text nodes which are a directly distributed, but I
may be incorrect.