[Cocoa] REGRESSION(r244182): Inspector thinks CA commits can be nested

[Said Abou-Hallawa]

With r244182, flushLayers can be scheduled immediately via RenderingUpdateScheduler::scheduleCompositingLayerFlush() or timed by the display monitor. This can lead nested CA commits. This means a CA commit begins before others commits finish.

WebInspector does not expect nested commits. When a commits begin it records a "willComposite". And when the composite finishes, it records a "didComposite". This can be fixed by coalescing nested composite as one recorded composite.

[Said Abou-Hallawa]

<rdar://problem/51079158>

[Said Abou-Hallawa]

Created attachment 371214 [details]
Patch

[Said Abou-Hallawa]

Created attachment 371235 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 371235 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:-1153
> -#if PLATFORM(COCOA)
> -    InspectorInstrumentation::willComposite(frame());
> -#endif

Doesn't iOS need this one?

> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:464
> +        [CATransaction addCommitHandler:[retainedPage = makeRefPtr(&m_webPage)] {
> +            if (Page* corePage = retainedPage->corePage()) {
> +                if (Frame* coreFrame = retainedPage->mainFrame())
> +                    corePage->inspectorController().willComposite(*coreFrame);
> +            }
> +        } forPhase:kCATransactionPhasePreCommit];

Another way to fix this bug would be ensure that we only call addCommitHandler:forPhase:kCATransactionPhasePreCommit once per commit.

[Devin Rousso]

Comment on attachment 371235 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:191
> +        if (m_startedComposites)

Should we be iterating `m_startedComposites` times instead?
```
    for (unsigned i = 0; i < m_startedComposites; ++i)
        didComposite();
```

This was first added in <https://trac.webkit.org/r186133> and has a pretty good explanation there.

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:360
> +    if (!m_startedComposites++)

Style: I'm not sure what WebKit style "mandates" for a situation like this, but I'd prefer it be separate.
```
    if (++m_startedComposites == 1)
```

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:366
> +    ASSERT(m_startedComposites);

I still think we should remove this `ASSERT`.  If Web Inspector connects in the middle of a composite, this would be triggered, which isn't what we want.  It's already the case that `didCompleteCurrentRecord` will early-return if there was no previous record to match.

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:367
> +    if (!--m_startedComposites)

Ditto (>360).
```
    if (--m_startedComposites == 0)
```

[Said Abou-Hallawa]

Created attachment 371378 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 371235 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:191
>> +        if (m_startedComposites)
> 
> Should we be iterating `m_startedComposites` times instead?
> ```
>     for (unsigned i = 0; i < m_startedComposites; ++i)
>         didComposite();
> ```
> 
> This was first added in <https://trac.webkit.org/r186133> and has a pretty good explanation there.

I made the change that Simon suggested. flushLayers() will process one preCommit/postCommit handler for nested CA commits. So m_startedComposite will not be changed from being a bool.

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:360
>> +    if (!m_startedComposites++)
> 
> Style: I'm not sure what WebKit style "mandates" for a situation like this, but I'd prefer it be separate.
> ```
>     if (++m_startedComposites == 1)
> ```

This function will not be changed.

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:366
>> +    ASSERT(m_startedComposites);
> 
> I still think we should remove this `ASSERT`.  If Web Inspector connects in the middle of a composite, this would be triggered, which isn't what we want.  It's already the case that `didCompleteCurrentRecord` will early-return if there was no previous record to match.

Assertion was removed.

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:367
>> +    if (!--m_startedComposites)
> 
> Ditto (>360).
> ```
>     if (--m_startedComposites == 0)
> ```

No need for this change anymore.

>> Source/WebCore/page/FrameView.cpp:-1153
>> -#endif
> 
> Doesn't iOS need this one?

Yes, you are right. Add a preCommit handler to RemoteLayerTreeDrawingArea::flushLayers() and TiledCoreAnimationDrawingArea::flushLayers(()

>> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:464
>> +        } forPhase:kCATransactionPhasePreCommit];
> 
> Another way to fix this bug would be ensure that we only call addCommitHandler:forPhase:kCATransactionPhasePreCommit once per commit.

Done.

[Devin Rousso]

Comment on attachment 371378 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:1941
> +    unsigned m_pendingComposites { 0 };

This means that every page is doing unnecessary work when Web Inspector isn't open.  Unless this is (or will be) used outside of Web Inspector, I'd rather this data be held by the `InspectorTimelineAgent`.

[Said Abou-Hallawa]

Comment on attachment 371378 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1941
>> +    unsigned m_pendingComposites { 0 };
> 
> This means that every page is doing unnecessary work when Web Inspector isn't open.  Unless this is (or will be) used outside of Web Inspector, I'd rather this data be held by the `InspectorTimelineAgent`.

The CA postCommit in TiledCoreAnimationDrawingArea::flushLayers() does more than calling didComposite(). It sends a message from the WebProcess to the UIProcess notifying it, a new painting milestone is reached. And I think we should be sending one message for nested commit as well.

[Simon Fraser (smfr)]

Comment on attachment 371378 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:374
> +    [CATransaction addCommitHandler:[retainedPage = makeRefPtr(&m_webPage)] {
> +        if (retainedPage->incrementPendingComposites() != 1)
> +            return;
> +        if (Page* corePage = retainedPage->corePage()) {
> +            if (Frame* coreFrame = retainedPage->mainFrame())
> +                corePage->inspectorController().willComposite(*coreFrame);
> +        }
> +    } forPhase:kCATransactionPhasePreCommit];
> +
> +    [CATransaction addCommitHandler:[retainedPage = makeRefPtr(&m_webPage)] {
> +        if (retainedPage->decrementPendingComposites())
> +            return;
> +        if (Page* corePage = retainedPage->corePage()) {
> +            if (Frame* coreFrame = retainedPage->mainFrame())
>                  corePage->inspectorController().didComposite(*coreFrame);
>          }

Why not just have a bit for "firstFlushAfterCommit", only call the addCommitHandlers when the bit is not set, and clear the bit in the post-commit handler?

[Said Abou-Hallawa]

Created attachment 371441 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 371441 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:331
> +    if (m_webPage.firstFlushAfterCommit())

Why not store the bit on RemoteLayerTreeDrawingArea?

[Said Abou-Hallawa]

Comment on attachment 371441 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:331
>> +    if (m_webPage.firstFlushAfterCommit())
> 
> Why not store the bit on RemoteLayerTreeDrawingArea?

Because this will require it to be stored on TiledCoreAnimationDrawingArea as well. If it is stored on the DrawingArea, the DrawingArea has to be retained and passed along with the WebPage to the postCommit handler.

   [CATransaction addCommitHandler:[retainedPage = makeRefPtr(&m_webPage), retainedDrawingArea = makeRefPtr(this)] {
        if (Page* corePage = retainedPage->corePage()) {
            if (Frame* coreFrame = retainedPage->mainFrame())
                corePage->inspectorController().didComposite(*coreFrame);
        }
        retainedDrawingArea->setFirstFlushAfterCommit(false);
    } forPhase:kCATransactionPhasePostCommit];

[WebKit Commit Bot]

Comment on attachment 371441 [details]
Patch

Clearing flags on attachment: 371441

Committed r246142: <https://trac.webkit.org/changeset/246142>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.