Bug 220832 – REGRESSION (r271514): [macOS] TestWebKitAPI.WebKit.PrintFrame is a flaky failure

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

TestWebKitAPI.WebKit.PrintFrame is a flaky failure on Catalina and Mojave release bots:

    TestWebKitAPI.WebKit.PrintFrame
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:420
        Expected equality of these values:
          operation.jobTitle.UTF8String
            Which is: ""
          "test_title"
        
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:393
        Expected equality of these values:
          title.UTF8String
            Which is: ""
          "test_title"
        

https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebKit.PrintFrame

[Ryan Haddad]

It looks like this started with https://trac.webkit.org/changeset/271514/webkit

[Radar WebKit Bug Importer]

<rdar://problem/73470910>

[Darin Adler]

This looks like a real failure. Now that the title is set lazily, WebKit delegate methods that put the title into the header or footer, might not see the title.

We would have to figure out what can eliminate that race. I don’t think the mistake is in the test, it’s a problem with WebKit API. We need to find a way to let that title get set before starting the print operation.

[Rob Buis]

Created attachment 418226 [details]
Patch

[Rob Buis]

Created attachment 418231 [details]
Patch

[Darin Adler]

Comment on attachment 418231 [details]
Patch

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

Interesting concepts. A few thoughts.

> Source/WebCore/dom/Document.cpp:1687
>      if (!m_updateTitleTaskScheduled) {
> +        incrementLoadEventDelayCount();
>          eventLoop().queueTask(TaskSource::DOMManipulation, [protectedThis = makeRef(*this), this]() mutable {
>              m_updateTitleTaskScheduled = false;
>              if (auto documentLoader = makeRefPtr(loader()))
>                  documentLoader->setTitle(m_title);
> +            decrementLoadEventDelayCount();
>          });
>          m_updateTitleTaskScheduled = true;
>      }

This seems like a change that could visibly slow down webpage loads, because dispatching the load event is delayed an additional event loop cycle. Or it might be harmless. I can’t tell just from reading the code.

I also worry that the count could end up permanently non-zero if there’s ever a way the task could be canceled. (Probably not a thing.)

> Source/WebCore/page/DOMWindow.cpp:1076
> -    if (frame->loader().activeDocumentLoader()->isLoading()) {
> +    if (frame->loader().activeDocumentLoader()->isLoading() || document()->parsing()) {

Inelegant that we have to check both. Is there some way to unify these concepts for the caller?

> Source/WebCore/page/DOMWindow.cpp:2327
> -    if (m_shouldPrintWhenFinishedLoading) {
> +    if (m_shouldPrintWhenFinishedLoading && !document()->parsing()) {

Is there a "finished parsing" callback?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:551
> -    webPage->send(Messages::WebPageProxy::DidReceiveTitleForFrame(m_frame->frameID(), truncatedTitle.string, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
> +    webPage->sendSync(Messages::WebPageProxy::DidReceiveTitleForFrame(m_frame->frameID(), truncatedTitle.string, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())),  Messages::WebPageProxy::DidReceiveTitleForFrame::Reply());

This will hurt performance. We don’t want the web process to have to wait for a reply. Consider that in many circumstances the title won’t even be displayed.

What’s the rationale?

[Rob Buis]

Comment on attachment 418231 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:1687
>>      }
> 
> This seems like a change that could visibly slow down webpage loads, because dispatching the load event is delayed an additional event loop cycle. Or it might be harmless. I can’t tell just from reading the code.
> 
> I also worry that the count could end up permanently non-zero if there’s ever a way the task could be canceled. (Probably not a thing.)

Right, I want to have something working first, and then would check the speed implications. Right, I don't think it can be canceled, though maybe if the load fails sometime after the task is scheduled but before it is processed.

>> Source/WebCore/page/DOMWindow.cpp:1076
>> +    if (frame->loader().activeDocumentLoader()->isLoading() || document()->parsing()) {
> 
> Inelegant that we have to check both. Is there some way to unify these concepts for the caller?

I am not sure yet this is needed. FWIW html spec ties printing processing to stopping the parser:
https://html.spec.whatwg.org/#stop-parsing

>> Source/WebCore/page/DOMWindow.cpp:2327
>> +    if (m_shouldPrintWhenFinishedLoading && !document()->parsing()) {
> 
> Is there a "finished parsing" callback?

I do not think so, I thought about tying this code to finished parsing instead indeed, but I do not know if anything would break for error loads.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:551
>> +    webPage->sendSync(Messages::WebPageProxy::DidReceiveTitleForFrame(m_frame->frameID(), truncatedTitle.string, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())),  Messages::WebPageProxy::DidReceiveTitleForFrame::Reply());
> 
> This will hurt performance. We don’t want the web process to have to wait for a reply. Consider that in many circumstances the title won’t even be displayed.
> 
> What’s the rationale?

I am not convinced now it is actually needed. I was trying to ensure that informing of the title to the UI process was done before handling printing in the UI process. But it looks like that does not depend on whether the message is send sync or async but rather the logic in DOMWindow.

[Rob Buis]

There are still questions left and multiple possible approaches so I am fine if anybody reverts for the time being.

I am wondering if we do need to block the load event just to keep test cases like printing-events.html working.

Another idea is for the printing code in either WebProcess or UIProcess to rely more on the document title as the source of the true title rather than waiting for the title task to be finished. But it feels a bit strange to special case title and UIProcess would not be able to reach the Document AFAIK.

So more research tomorrow.

[Rob Buis]

Created attachment 418235 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 418235 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:1076
> -    if (frame->loader().activeDocumentLoader()->isLoading()) {
> +    if (frame->loader().activeDocumentLoader()->isLoading() || document()->updateTitleTaskScheduled()) {

Rather than delaying the print like this, we should just execute DocumentLoader::setTitle here instead.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:551
> +    webPage->sendSync(Messages::WebPageProxy::DidReceiveTitleForFrame(m_frame->frameID(), truncatedTitle.string, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())), Messages::WebPageProxy::DidReceiveTitleForFrame::Reply());

We definitely don't want to make more IPC's sync!

[Ryosuke Niwa]

Let's revert the original patch (r271514) for now so that we can take our time to fix this problem.

[Ryosuke Niwa]

Reverted the patch in https://trac.webkit.org/r271878