Bug 155503 – Delay HTMLFormControlElement::focus() call until after layout is finished.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

If we are in the middle of layout.

[alan]

rdar://problem/24046635

[alan]

Created attachment 274112 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 274112 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Covered by LayoutTests/fast/dom/adopt-node-crash-2.html

Can we make a test that triggered the bad behavior on Mac (i.e. one that enables frame flattening)?

> Source/WebCore/page/FrameView.cpp:3097
> +    for (size_t i = 0; i < m_postLayoutCallbackQueue.size(); ++i)
> +        m_postLayoutCallbackQueue[i]();

I think you should copy the vector, in case a callback modifies it.

[alan]

Created attachment 274127 [details]
Patch

[Build Bot]

Comment on attachment 274127 [details]
Patch

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

New failing tests:
js/function-call-aliased.html

[Build Bot]

Created attachment 274137 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[alan]

Committed r198238: <http://trac.webkit.org/changeset/198238>

[Darin Adler]

Comment on attachment 274127 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3089
> +void FrameView::queuePostLayoutCallback(std::function<void()> callback)
> +{
> +    m_postLayoutCallbackQueue.append(callback);
> +}

Should use std::function<void()>&& for the argument type and use WTFMove.

> Source/WebCore/page/FrameView.cpp:3102
> +    if (!m_postLayoutCallbackQueue.size())
> +        return;
> +
> +    const auto queue = m_postLayoutCallbackQueue;
> +    m_postLayoutCallbackQueue.clear();
> +    for (size_t i = 0; i < queue.size(); ++i)
> +        queue[i]();

Here’s how I’d write this, using a modern for loop and move:

    auto queue = WTFMove(m_postLayoutCallbackQueue);
    for (auto& task : queue)
        task();

No need for a special case for size zero, since it’s already super efficient without a special case. No need to copy and then clear; just move and allocate a new one later when it’s needed.

[Darin Adler]

Comment on attachment 274127 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:3102
>> +        queue[i]();
> 
> Here’s how I’d write this, using a modern for loop and move:
> 
>     auto queue = WTFMove(m_postLayoutCallbackQueue);
>     for (auto& task : queue)
>         task();
> 
> No need for a special case for size zero, since it’s already super efficient without a special case. No need to copy and then clear; just move and allocate a new one later when it’s needed.

One other thought:

In other places where we queue up operations like this, we typically define the semantics for what happens if more operations are queued up during processing. In this case, what will happen is that any tasks that are queued up late will be saved and executed on the next layout. I’m not sure that’s the behavior we want. We should consider one of these:

1) Making it illegal to queue up tasks at this point (so putting an assertion into the queue function), if it’s practical to live with this constraint.

2) Coding it so that any additionally added tasks will be executed. One good way to do that it to iterate using indexes, not make a copy, and clear when done. That way, anything added will be covered by the iteration. I think that’s what your patch did before Simon gave you his feedback.

Maybe there’s some other good way to have it behave.

[alan]

(In reply to comment #9)
> Comment on attachment 274127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274127&action=review
> 
> >> Source/WebCore/page/FrameView.cpp:3102
> >> +        queue[i]();
> > 
> > Here’s how I’d write this, using a modern for loop and move:
> > 
> >     auto queue = WTFMove(m_postLayoutCallbackQueue);
> >     for (auto& task : queue)
> >         task();
> > 
> > No need for a special case for size zero, since it’s already super efficient without a special case. No need to copy and then clear; just move and allocate a new one later when it’s needed.
> 
> One other thought:
> 
> In other places where we queue up operations like this, we typically define
> the semantics for what happens if more operations are queued up during
> processing. In this case, what will happen is that any tasks that are queued
> up late will be saved and executed on the next layout. I’m not sure that’s
> the behavior we want. We should consider one of these:
> 
> 1) Making it illegal to queue up tasks at this point (so putting an
> assertion into the queue function), if it’s practical to live with this
> constraint.
> 
> 2) Coding it so that any additionally added tasks will be executed. One good
> way to do that it to iterate using indexes, not make a copy, and clear when
> done. That way, anything added will be covered by the iteration. I think
> that’s what your patch did before Simon gave you his feedback.
> 
> Maybe there’s some other good way to have it behave.
The many different ways of triggering FrameView::performPostLayoutTasks() makes post layout tasks a bit confusing and it clearly needs some cleanup, but if we ignore that for a moment, I think the second approach is the way to go. If executing a particular post layout task initiates a sync layout and during this (nested) layout we decide to queue up some additional tasks, then we should definitely run those tasks in the same cycle as there might not be a next layout. Fortunately when FrameView::performPostLayoutTasks() is called on a timer and flushing the queue triggers this extra layout (which is not considered nested layout at this point), we already have this m_inSynchronousPostLayout flag to prevent us from running the flushAnyPendingPostLayoutTasks() on the same callstack. (it provides similar protection as the m_nestedLayoutCount)
This works fine as long as the layout triggered by the queued task is synchronous. However async layouts (where we just schedule a new layout) should never end up queuing tasks for the current layout. (not sure how to assert on that though)

[alan]

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