Bug 104829 – Introduce a mechanism to run a task before width computation or layout and migrate RenderCounter to using it

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Julien Chaffraix]

Mutating the tree during layout has been the source of a lot of security vulnerabilities over the past year. This mechanism would give the renderers that currently mutate the tree during layout a safe way to do so as a pre-layout phase. This should significantly reduce the risk of confusing the layout logic.

Another upside is that it would also make it possible to enforce a strict no-tree-mutation-during-layout policy that would be good to catch up future offenders.

Current renderers that could benefit from such a change:
* RenderCounter (calling updateText during computePreferredLogicalWidths)
* RenderQuote (calling attachQuote during computePreferredLogicalWidths)
* RenderListItem (calling updateMarkerLocation during computePreferredLogicalWidths and layout)
* first-letter in RenderBlock and RenderTableCell (calling updateFirstLetter during computePreferredLogicalWidths and layout)
* RenderMathMLOperator (calling updateFromElement during computePreferredLogicalWidths). Note: We won't be able to remove any mutation for this renderer as RenderMathMLOperator::stretchToHeight requires the operator to rebuild its "glyph" subtree.

[Eric Seidel (no email)]

So is this a whole new phase?  A two-pass layout of sort?  It would help me to understand what is vs. is not OK to put in this phase. :)

[Julien Chaffraix]

(In reply to comment #1)
> So is this a whole new phase?  A two-pass layout of sort?  It would help me to understand what is vs. is not OK to put in this phase. :)

It's really meant to be a mutation phase before layout so that we operate on a fixed tree. It should stay simple (complexity is what makes it hard to allow any tree mutation during layout) so we should restrict it to mutation tasks only.

[Eric Seidel (no email)]

I like it.  Would we implement this with a separate dirty bit, and a separate tree-walk? (like style resolve), or share the layout dirty-bit and possibly even the tree-walk (like how paint does its 7-phase burrito).

[Abhishek Arya]

Julien, thanks for taking this up. This is pretty important one and in the past, we have just trying to patch these bugs by pushing tree mutation during layout, but it still creeps in many places. So, this will help to push those things over a new phase before layout.

[Julien Chaffraix]

> Would we implement this with a separate dirty bit, and a separate tree-walk? (like style resolve), or share the layout dirty-bit and possibly even the tree-walk (like how paint does its 7-phase burrito).

What I am considering is an opt-in mechanism where you register a task to run before layout (I consider the mechanism different from . I was going to post a patch in this direction using global HashMap but the lack of mutation safe iterators made me wonder if I am not trading a known issue for a new one down the road.

A separate tree-walk is an overkill as I would expect the renderers mentioned in the first comment are rare or at least sparse in the tree.

[Eric Seidel (no email)]

(In reply to comment #5)
> What I am considering is an opt-in mechanism where you register a task to run before layout (I consider the mechanism different from . I was going to post a patch in this direction using global HashMap but the lack of mutation safe iterators made me wonder if I am not trading a known issue for a new one down the road.

That sounds a lot like the post-attach callback mechanism, which I'm not a big fan of.  I find post-attach callbacks cause very difficult to understand control-flow.

(Perhaps naively) I'd rather do a full-tree-walk when we need this phase, if the appropriate dirty-bit is set.

[Julien Chaffraix]

(In reply to comment #6)
> (In reply to comment #5)
> > What I am considering is an opt-in mechanism where you register a task to run before layout (I consider the mechanism different from . I was going to post a patch in this direction using global HashMap but the lack of mutation safe iterators made me wonder if I am not trading a known issue for a new one down the road.
> 
> That sounds a lot like the post-attach callback mechanism, which I'm not a big fan of.  I find post-attach callbacks cause very difficult to understand control-flow.

I don't deny the readability issue for which I don't have a good solution.

> (Perhaps naively) I'd rather do a full-tree-walk when we need this phase, if the appropriate dirty-bit is set.

The catch of this approach is that we need to either reuse a dirty-bit on RenderObject (which involves a loss of readability too) or free an existing one (challenging but probably possible). This bit will be also mostly unused if we assume that the occurrence of such renderer is low.

[Simon Fraser (smfr)]

(In reply to comment #7)
> The catch of this approach is that we need to either reuse a dirty-bit on RenderObject (which involves a loss of readability too) or free an existing one (challenging but probably possible). This bit will be also mostly unused if we assume that the occurrence of such renderer is low.

Yeah, I'd prefer to avoid another set of dirty bits and a full tree walk.

[Eric Seidel (no email)]

Doesn't paint walk the tree 7 times already? :)
http://www.w3.org/TR/CSS2/zindex.html#painting-order

[Julien Chaffraix]

Created attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

[Peter Beverloo (cr-android ews)]

Comment on attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

Attachment 179844 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15378903

[WebKit Review Bot]

Comment on attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

Attachment 179844 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15402013

New failing tests:
css2.1/t1202-counter-01-b.html
css2.1/t1202-counter-08-b.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counter-07-b.html
editing/selection/home-end.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
http/tests/security/view-source-no-refresh.html
css2.1/t1202-counter-15-b.html
css2.1/t1202-counters-09-b.html
css2.1/t1202-counters-11-b.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
css2.1/t1202-counters-07-b.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css2.1/t1202-counters-01-b.html
http/tests/security/view-source-no-javascript-url.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counter-03-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html

[Build Bot]

Comment on attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

Attachment 179844 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15372989

[EFL EWS Bot]

Comment on attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

Attachment 179844 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15412009

[Julien Chaffraix]

Comment on attachment 179844 [details]
Proposed fix: Took Simon & Eric comments into account -> full tree traversal if we have a task, use of an out-of-band mechanism.

Investigating what went wrong.

[Julien Chaffraix]

Created attachment 180014 [details]
Proposed change 2: Fixed a dump mistake when switching to tree walking, should pass the tests this time.

[Elliott Sprehn]

Comment on attachment 180014 [details]
Proposed change 2: Fixed a dump mistake when switching to tree walking, should pass the tests this time.

View in context: https://bugs.webkit.org/attachment.cgi?id=180014&action=review

I can't r- this, but I don't think this is right. You're going to execute the mutating tasks inside layout from child frames.

> Source/WebCore/rendering/RenderCounter.cpp:35
> +#include <wtf/Functional.h>

Why do we need functional? It seems this is not needed anymore.

> Source/WebCore/rendering/RenderObject.cpp:462
> +void RenderObject::executePreLayoutTreeMutatingTasks()

This should be static.

> Source/WebCore/rendering/RenderObject.h:221
> +    void executePreLayoutTreeMutatingTasks();

static. And should be called as RenderObject::executePreLayoutTreeMutatingTasks() since it affects every document's render tree, not just the one that's about to layout. This still seems rather dangerous to be honest since inside ::layout() if we call ::layout() on a child frame that would execute all the mutating tasks on the parent render tree which is bad. I think this happens for seamless iframes.

[Elliott Sprehn]

(In reply to comment #17)
> (From update of attachment 180014 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180014&action=review
> 
> I can't r- this, but I don't think this is right. You're going to execute the mutating tasks inside layout from child frames.

Oh I see, you walk the entire render tree doing hash set lookup for every renderer to avoid the badness. This is going to be crazy slow for counters and quotes. On the HTML5 spec you just introduced a walk of tens of thousands of renderers right before layout.

[Eric Seidel (no email)]

Yes. Seamless is one of the ways you can end up laying out / style resolving your ancestors before you. Its not safe to assume subtree layouts are idempotent.  :)

[Elliott Sprehn]

(In reply to comment #19)
> Yes. Seamless is one of the ways you can end up laying out / style resolving your ancestors before you. Its not safe to assume subtree layouts are idempotent.  :)

Yeah, I just noticed that Julien has a full tree walk in there to prevent the badness I had described:

for (RenderObject* object = this; object; object = object->nextInPreOrder(this)) {
  if (gPreLayoutTreeMutatingTasks->contains(object))
      object->doPreLayoutTreeMutatingTask();
}

and this shouldn't be static, my bad for not looking closer. :)

Still this seems like it'll be a huge perf hit for html5-full-parse.html since we're walking tens of thousands of renderers and doing a hashtable lookup for each one as soon as there's a single <q> in there.

[Abhishek Arya]

(In reply to comment #20)
> (In reply to comment #19)
> > Yes. Seamless is one of the ways you can end up laying out / style resolving your ancestors before you. Its not safe to assume subtree layouts are idempotent.  :)
> 
> Yeah, I just noticed that Julien has a full tree walk in there to prevent the badness I had described:
> 
> for (RenderObject* object = this; object; object = object->nextInPreOrder(this)) {
>   if (gPreLayoutTreeMutatingTasks->contains(object))
>       object->doPreLayoutTreeMutatingTask();
> }
> 
> and this shouldn't be static, my bad for not looking closer. :)
> 
> Still this seems like it'll be a huge perf hit for html5-full-parse.html since we're walking tens of thousands of renderers and doing a hashtable lookup for each one as soon as there's a single <q> in there.

I think it will be way faster to do it the other way around, go over all the renderers in the hashset and see if they are a descendant of |this|. I hope the order does not matter.

[Elliott Sprehn]

(In reply to comment #21)
> ...
> 
> I think it will be way faster to do it the other way around, go over all the renderers in the hashset and see if they are a descendant of |this|. I hope the order does not matter.

That would certainly be better. We could also optimize it so that if the |this| is the RenderView then we just check renderer->view() == this instead of walking up through every parent.

[Julien Chaffraix]

> > I think it will be way faster to do it the other way around, go over all the renderers in the hashset and see if they are a descendant of |this|. I hope the order does not matter.
> 
> That would certainly be better. We could also optimize it so that if the |this| is the RenderView then we just check renderer->view() == this instead of walking up through every parent.

It seems that what you are asking for is what I was proposing initially (see comment #5): basically register a renderer for a pre-layout task and execute the task just on them. Eric was against this idea due to readability concerns and I misread Simon's comment, thinking he agreed with Eric.
It may be possible to attach the tasks to the RenderView and run them from there, avoiding the need for the RenderView checks you mentioned.

[Julien Chaffraix]

Created attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

[Antonio Gomes]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review

> Source/WebCore/rendering/RenderView.cpp:217
> +bool RenderView::isRegisteredForPreLayoutTask(RenderObject* object)

const?

[Elliott Sprehn]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review

Awesome! At some point we may consider sorting the HashSet in tree order as it would make implementing Counters or Quotes faster and easier, but we can do that later.

> Source/WebCore/ChangeLog:11
> +        Refactoring, no expected change in behavior (apart from counter not mutating the tree during layout).

You might note that this doesn't execute the tree mutating tasks in tree order, but in registration order.

> Source/WebCore/rendering/RenderView.cpp:217
> +bool RenderView::isRegisteredForPreLayoutTask(RenderObject* object)

Missing const for the method

> Source/WebCore/rendering/RenderView.cpp:244
> +        RenderObject* currentRenderer = renderers[i];

You might add a comment here that currentRenderer could have already been free'd() because of a previous call to executePreLayoutTreeMutatingTask().

> Source/WebCore/rendering/RenderView.h:232
> +    bool isRegisteredForPreLayoutTask(RenderObject*);

const;

[Darin Adler]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review

> Source/WebCore/rendering/RenderView.cpp:247
> +    Vector<RenderObject*> renderers;
> +    copyToVector(*m_preLayoutTreeMutatingTasks, renderers);
> +    for (size_t i = 0; i < renderers.size(); ++i) {
> +        RenderObject* currentRenderer = renderers[i];
> +        if (m_preLayoutTreeMutatingTasks->contains(currentRenderer))
> +            currentRenderer->executePreLayoutTreeMutatingTask();
> +    }

Is it safe to do these in an unpredictable, hash table order? If we want to do them in registration order we need to use a ListHashSet instead of a HashSet.

[Elliott Sprehn]

(In reply to comment #26)
> (From update of attachment 182797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review
> ...
> 
> You might note that this doesn't execute the tree mutating tasks in tree order, but in registration order.

I noticed I was wrong, but forgot to remove this comment. HashSet is effectively random order, ListHashSet would be registration order. 

It's probably better to use HashSet for now since it keeps people from depending on the order since the initial render tree construction on page load would produce tree order from registration order, but then doing an appendChild(...)  of a new counter would place it out of order (on the end). I like having this imposed randomness unless we plan to sort in tree order.

[Build Bot]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

Attachment 182797 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15900086

[Peter Beverloo (cr-android ews)]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

Attachment 182797 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15900090

[Julien Chaffraix]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review

>>> Source/WebCore/rendering/RenderView.cpp:217
>>> +bool RenderView::isRegisteredForPreLayoutTask(RenderObject* object)
>> 
>> const?
> 
> Missing const for the method

Totally missed this one, will fix (along with the build).

>> Source/WebCore/rendering/RenderView.cpp:244
>> +        RenderObject* currentRenderer = renderers[i];
> 
> You might add a comment here that currentRenderer could have already been free'd() because of a previous call to executePreLayoutTreeMutatingTask().

Sure to both comments.

>> Source/WebCore/rendering/RenderView.cpp:247
>> +    }
> 
> Is it safe to do these in an unpredictable, hash table order? If we want to do them in registration order we need to use a ListHashSet instead of a HashSet.

Both orders are arbitrary as we ideally should follow a tree ordering. As the pre-layout tasks are very self-contained (ie touch one renderer) and we properly check for deletion, a random order should be fine.

[Ryosuke Niwa]

(In reply to comment #31)
> (From update of attachment 182797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review
>
> >> Source/WebCore/rendering/RenderView.cpp:247
> >> +    }
> > 
> > Is it safe to do these in an unpredictable, hash table order? If we want to do them in registration order we need to use a ListHashSet instead of a HashSet.
> 
> Both orders are arbitrary as we ideally should follow a tree ordering. As the pre-layout tasks are very self-contained (ie touch one renderer) and we properly check for deletion, a random order should be fine.

Is there a way to enforce that? Or can we change function/variable names to reflect this intention? It's not obvious from your patch that pre layout mutation is supposed to be constrained within one renderer.

[EFL EWS Bot]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

Attachment 182797 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15908003

[Julien Chaffraix]

Comment on attachment 182797 [details]
Updated fix: Avoids the tree iteration, the mechanism hangs off the RenderView (like the counter counting mechanism).

View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review

>>>> Source/WebCore/rendering/RenderView.cpp:247
>>>> +    }
>>> 
>>> Is it safe to do these in an unpredictable, hash table order? If we want to do them in registration order we need to use a ListHashSet instead of a HashSet.
>> 
>> Both orders are arbitrary as we ideally should follow a tree ordering. As the pre-layout tasks are very self-contained (ie touch one renderer) and we properly check for deletion, a random order should be fine.
> 
> Is there a way to enforce that? Or can we change function/variable names to reflect this intention? It's not obvious from your patch that pre layout mutation is supposed to be constrained within one renderer.

My wording was wrong: some tasks are self-contained (e.g RenderCounter) but other are not (like RenderMathMLOperation or RenderQuote). That's also why we need to handle deletion in the loop above.

Enforcing self-containment would be artificial and reduce the usefulness of this code.

[Ryosuke Niwa]

(In reply to comment #34)
> (From update of attachment 182797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182797&action=review
>
> My wording was wrong: some tasks are self-contained (e.g RenderCounter) but other are not (like RenderMathMLOperation or RenderQuote). That's also why we need to handle deletion in the loop above.
> 
> Enforcing self-containment would be artificial and reduce the usefulness of this code.

If that were the case, why is it okay to execute these tasks in a random order?

[Julien Chaffraix]

(Replying inline as the review tool didn't show your comment, not sure why)

> > Enforcing self-containment would be artificial and reduce the usefulness of this code.
> 
> If that were the case, why is it okay to execute these tasks in a random order?

It is fine to do a random order execution as we iterate over a frozen set ensuring the objects are still alive when executing the task. We thought of other bugs that could be traversal dependent but couldn't think of others. If there are other bugs, we can ensure that the renderers are ordered.
Note that random order could end up doing some extra-work (e.g. if you process the children and then blow them off when you reach the parent) but that shouldn't be much of a big deal as these conditions should be rare.

[Julien Chaffraix]

Created attachment 182878 [details]
Patch v4: Fixed release build and integrated all the comments.

[Julien Chaffraix]

Created attachment 182883 [details]
Patch v5: Rebaselined for the EWS.

[Ryosuke Niwa]

(In reply to comment #36)
>
> Note that random order could end up doing some extra-work (e.g. if you process the children and then blow them off when you reach the parent) but that shouldn't be much of a big deal as these conditions should be rare.

That sounds extremely bad. What if we had a bug in that rare cases? We'll end up having a non-deterministic test failures.

[Julien Chaffraix]

(In reply to comment #39)
> (In reply to comment #36)
> >
> > Note that random order could end up doing some extra-work (e.g. if you process the children and then blow them off when you reach the parent) but that shouldn't be much of a big deal as these conditions should be rare.
> 
> That sounds extremely bad. What if we had a bug in that rare cases? We'll end up having a non-deterministic test failures.

I have a hard time figuring your point here. Are you arguing to use a ListHashSet to get a deterministic traversal or is it something else?

[Abhishek Arya]

Comment on attachment 182883 [details]
Patch v5: Rebaselined for the EWS.

View in context: https://bugs.webkit.org/attachment.cgi?id=182883&action=review

Lets use the ListHashSet for deterministic order in traversal and predictable failures.

> Source/WebCore/rendering/RenderView.cpp:218
> +        m_preLayoutTreeMutatingTasks = adoptPtr(new HashSet<RenderObject*>());

Don't we have a typedef for ListHashSet<RenderObject*> ?

[Ojan Vafai]

Comment on attachment 182883 [details]
Patch v5: Rebaselined for the EWS.

Are we guaranteed that computePreferredLogicalWidths is only called from within layout? If not, you could end up in a case where computePreferredLogicalWidths computes the wrong values because the pre layout tasks haven't been run.

[Ojan Vafai]

*** Bug 107958 has been marked as a duplicate of this bug. ***

[Elliott Sprehn]

(In reply to comment #41)
> (From update of attachment 182883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182883&action=review
> 
> Lets use the ListHashSet for deterministic order in traversal and predictable failures.
> 

If we go this route we need to be really careful about test coverage. Lots of tests are going to end up running the tasks in tree order because they just use the render tree as it resulted from the page load.

Maybe we can use mod or something to predictably shuffle the elements out of tree order, or just make sure it's always sorted in tree order.

I liked using HashSet since it prevents people from depending on this, but it does make test failures hard to debug.

[Ojan Vafai]

I'll try adding an assert that we're in layout when computing preferred widths and see what breaks. :)

[Ojan Vafai]

Looks like we hit this assert a lot. Will dig into the stack to see if this is easily fixable. Until this is fixed, this patch isn't quite correct though.

[Julien Chaffraix]

(In reply to comment #46)
> Looks like we hit this assert a lot. Will dig into the stack to see if this is easily fixable. Until this is fixed, this patch isn't quite correct though.

Weird, I did some manual checking (not as good as your ASSERT) and found one violation: RenderMarquee which pokes its box's preferred logical widths during scroll (which should be fine as we expect layout information to be accurate).

(In reply to comment #44)
> (In reply to comment #41)
> > (From update of attachment 182883 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182883&action=review
> > 
> > Lets use the ListHashSet for deterministic order in traversal and predictable failures.
> > 
> 
> If we go this route we need to be really careful about test coverage. Lots of tests are going to end up running the tasks in tree order because they just use the render tree as it resulted from the page load.
> 
> Maybe we can use mod or something to predictably shuffle the elements out of tree order, or just make sure it's always sorted in tree order.
> 
> I liked using HashSet since it prevents people from depending on this, but it does make test failures hard to debug.

I would rather keep that for a follow-up patch if we see the need for it. Also I would expect our fuzzers to pick up out of order traversal if it causes an issue.

[Ojan Vafai]

(In reply to comment #47)
> (In reply to comment #46)
> > Looks like we hit this assert a lot. Will dig into the stack to see if this is easily fixable. Until this is fixed, this patch isn't quite correct though.
> 
> Weird, I did some manual checking (not as good as your ASSERT) and found one violation: RenderMarquee which pokes its box's preferred logical widths during scroll (which should be fine as we expect layout information to be accurate).

I'm working on refining the assert. I think this patch is fine to go in and we can just fix any violations that would be a problem. For example, the most common violation is in RenderImage, which is fine as long as we don't add a pre-layout hook to RenderImage.

[Elliott Sprehn]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > Looks like we hit this assert a lot. Will dig into the stack to see if this is easily fixable. Until this is fixed, this patch isn't quite correct though.
> > 
> > Weird, I did some manual checking (not as good as your ASSERT) and found one violation: RenderMarquee which pokes its box's preferred logical widths during scroll (which should be fine as we expect layout information to be accurate).
> 
> I'm working on refining the assert. I think this patch is fine to go in and we can just fix any violations that would be a problem. For example, the most common violation is in RenderImage, which is fine as long as we don't add a pre-layout hook to RenderImage.

Can we add an ASSERT(!renderer->isImage()) in the register function for the hook with a FIXME?

[Ojan Vafai]

Comment on attachment 182883 [details]
Patch v5: Rebaselined for the EWS.

View in context: https://bugs.webkit.org/attachment.cgi?id=182883&action=review

> Source/WebCore/rendering/RenderCounter.h:52
> +    virtual void executePreLayoutTreeMutatingTask() OVERRIDE;

I think this name needs to change. Really this needs to run before we compute intrinsic widths. For example, see RenderIFrame::maxPreferredLogicalWidth. We need to run this on childRoot before we run childRoot->maxPreferredLogicalWidth(). There's no layout involved.

Same thing applies to the documentView->minPreferredLogicalWidth() call in FrameView::autoSizeIfEnabled.

prepareTreeForWidthComputation ?

[Julien Chaffraix]

Comment on attachment 182883 [details]
Patch v5: Rebaselined for the EWS.

View in context: https://bugs.webkit.org/attachment.cgi?id=182883&action=review

>> Source/WebCore/rendering/RenderCounter.h:52
>> +    virtual void executePreLayoutTreeMutatingTask() OVERRIDE;
> 
> I think this name needs to change. Really this needs to run before we compute intrinsic widths. For example, see RenderIFrame::maxPreferredLogicalWidth. We need to run this on childRoot before we run childRoot->maxPreferredLogicalWidth(). There's no layout involved.
> 
> Same thing applies to the documentView->minPreferredLogicalWidth() call in FrameView::autoSizeIfEnabled.
> 
> prepareTreeForWidthComputation ?

Fine with the new name (not super happy that it doesn't underline that the tasks are tree mutating but I could live with that).

>> Source/WebCore/rendering/RenderView.cpp:218
>> +        m_preLayoutTreeMutatingTasks = adoptPtr(new HashSet<RenderObject*>());
> 
> Don't we have a typedef for ListHashSet<RenderObject*> ?

No, there is none.

> Source/WebCore/rendering/RenderView.cpp:235
> +    copyToVector(*m_preLayoutTreeMutatingTasks, renderers);

Using ListHashSet will require implementing copyToVector (just a FYI).

[Ojan Vafai]

Was talking to Emil and Levi and we had some more thoughts on this. While this is a clear improvement over our current state of affairs, the ideal solution would be to not do any of the tree mutating at all (e.g. do it during styleDidChange/childrenChanged or something like that). Given that, we should try to make it as clear as possible that new code could not be adding treemutating tasks. Maybe we could make the naming clear that it's only for quotes and counters? Or maybe we could have the register function assert that the object it gets is a quote/counter? Is there anything other than quotes/counters we want this for?

(In reply to comment #51)
> > prepareTreeForWidthComputation ?
> 
> Fine with the new name (not super happy that it doesn't underline that the tasks are tree mutating but I could live with that).

I'd be fine with an even more verbose name. I just think it needs to be clear that it's pre width computation, not just pre layout.

[Ojan Vafai]

How's this for verbose: mutateTreeBeforeWidthComputationOrLayout :)

[Julien Chaffraix]

(In reply to comment #52)
> Was talking to Emil and Levi and we had some more thoughts on this. While this is a clear improvement over our current state of affairs, the ideal solution would be to not do any of the tree mutating at all (e.g. do it during styleDidChange/childrenChanged or something like that).

The idea is really to start enforcing a no-tree-mutation policy, which would effectively prevent new violations.

> Given that, we should try to make it as clear as possible that new code could not be adding treemutating tasks. Maybe we could make the naming clear that it's only for quotes and counters? Or maybe we could have the register function assert that the object it gets is a quote/counter? Is there anything other than quotes/counters we want this for?

From the description (comment 0), here is the (non-exhaustive) list:
* RenderCounter (calling updateText during computePreferredLogicalWidths)
* RenderQuote (calling attachQuote during computePreferredLogicalWidths)
* first-letter in RenderBlock and RenderTableCell (calling updateFirstLetter during computePreferredLogicalWidths and layout)
* RenderMathMLOperator (calling updateFromElement during computePreferredLogicalWidths). 
[You removed the tree mutation in RenderListItem since this bug was open.]

We could probably add an ASSERT but we will have to be careful to avoid weakening it (RenderBlocks should only have a pre-width computation task if they have first-letter set).

(In reply to comment #53)
> How's this for verbose: mutateTreeBeforeWidthComputationOrLayout :)

I like it better than the previous version, it also conveys the idea that pre-width computation hooks (a bit dry).

[Elliott Sprehn]

(In reply to comment #52)
> Was talking to Emil and Levi and we had some more thoughts on this. While this is a clear improvement over our current state of affairs, the ideal solution would be to not do any of the tree mutating at all (e.g. do it during styleDidChange/childrenChanged or something like that). 
> 

Yes, I have plans to move more and more of this into recalcStyle() hooks.

[Elliott Sprehn]

(In reply to comment #55)
> (In reply to comment #52)
> > Was talking to Emil and Levi and we had some more thoughts on this. While this is a clear improvement over our current state of affairs, the ideal solution would be to not do any of the tree mutating at all (e.g. do it during styleDidChange/childrenChanged or something like that). 
> > 
> 
> Yes, I have plans to move more and more of this into recalcStyle() hooks.

First step of RenderQuote: https://bugs.webkit.org/show_bug.cgi?id=109876

[Julien Chaffraix]

We discussed this patch with Ojan and Elliot and based on the fact that computePreferredLogicalWidths is called outside layout, the assumptions that this patch make are wrong. There is no clear path to fix the patch and it would probably be better to just fix the offending call sites instead of introducing the concept of pre-layout hooks in the first place.

Marking as WONTFIX based on the previous explanations.