Bug 155503 - Delay HTMLFormControlElement::focus() call until after layout is finished.
Summary: Delay HTMLFormControlElement::focus() call until after layout is finished.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
: 141683 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-03-15 12:31 PDT by zalan
Modified: 2016-03-25 15:49 PDT (History)
2 users (show)

See Also:


Attachments
Patch (12.23 KB, patch)
2016-03-15 12:46 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2016-03-15 14:17 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (947.02 KB, application/zip)
2016-03-15 15:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2016-03-15 12:31:02 PDT
If we are in the middle of layout.
Comment 1 zalan 2016-03-15 12:31:18 PDT
rdar://problem/24046635
Comment 2 zalan 2016-03-15 12:46:24 PDT
Created attachment 274112 [details]
Patch
Comment 3 Simon Fraser (smfr) 2016-03-15 13:41:40 PDT
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.
Comment 4 zalan 2016-03-15 14:17:54 PDT
Created attachment 274127 [details]
Patch
Comment 5 Build Bot 2016-03-15 15:03:58 PDT
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
Comment 6 Build Bot 2016-03-15 15:04:01 PDT
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
Comment 7 zalan 2016-03-15 16:10:14 PDT
Committed r198238: <http://trac.webkit.org/changeset/198238>
Comment 8 Darin Adler 2016-03-16 14:54:45 PDT
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.
Comment 9 Darin Adler 2016-03-16 14:59:20 PDT
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.
Comment 10 zalan 2016-03-17 22:10:52 PDT
(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)
Comment 11 zalan 2016-03-25 15:49:34 PDT
*** Bug 141683 has been marked as a duplicate of this bug. ***