Bug 198497

Summary: [Cocoa] REGRESSION(r244182): Inspector thinks CA commits can be nested
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, drousso, ews-watchlist, joepeck, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=177484
Bug Depends on:    
Bug Blocks: 198639    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-06-03 14:16:11 PDT
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.
Comment 1 Said Abou-Hallawa 2019-06-03 14:37:14 PDT
<rdar://problem/51079158>
Comment 2 Said Abou-Hallawa 2019-06-03 14:42:19 PDT
Created attachment 371214 [details]
Patch
Comment 3 Said Abou-Hallawa 2019-06-03 18:18:24 PDT
Created attachment 371235 [details]
Patch
Comment 4 Simon Fraser (smfr) 2019-06-04 08:18:00 PDT
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.
Comment 5 Devin Rousso 2019-06-04 09:30:57 PDT
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)
```
Comment 6 Said Abou-Hallawa 2019-06-04 22:51:23 PDT
Created attachment 371378 [details]
Patch
Comment 7 Said Abou-Hallawa 2019-06-05 08:31:11 PDT
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.
Comment 8 Devin Rousso 2019-06-05 10:15:04 PDT
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`.
Comment 9 Said Abou-Hallawa 2019-06-05 10:23:29 PDT
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.
Comment 10 Simon Fraser (smfr) 2019-06-05 13:43:10 PDT
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?
Comment 11 Said Abou-Hallawa 2019-06-05 15:02:37 PDT
Created attachment 371441 [details]
Patch
Comment 12 Simon Fraser (smfr) 2019-06-05 15:12:05 PDT
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?
Comment 13 Said Abou-Hallawa 2019-06-05 15:49:40 PDT
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];
Comment 14 WebKit Commit Bot 2019-06-05 20:39:14 PDT
Comment on attachment 371441 [details]
Patch

Clearing flags on attachment: 371441

Committed r246142: <https://trac.webkit.org/changeset/246142>
Comment 15 WebKit Commit Bot 2019-06-05 20:39:16 PDT
All reviewed patches have been landed.  Closing bug.