Bug 204199 – Reduce structure copies when creating an API::FrameInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

WKFrameInfo.webView should not return nil

[Alex Christensen]

Created attachment 383568 [details]
Patch

[Alex Christensen]

Created attachment 383569 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 383577 [details]
Patch

[Alex Christensen]

Created attachment 383579 [details]
Patch

[Brady Eidson]

Comment on attachment 383579 [details]
Patch

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

Could I please convince you to split the intended behavior change from all of the argument passing cleanup?

Seems quite unrelated

> Source/WebKit/UIProcess/API/C/WKFrame.cpp:141
>  WKFrameInfoRef WKFrameCreateFrameInfo(WKFrameRef frameRef)
>  {
> -    return toAPI(&API::FrameInfo::create(*toImpl(frameRef), WebCore::SecurityOrigin::createFromString(toImpl(frameRef)->url())).leakRef());
> +    auto* page = toImpl(frameRef)->page();
> +    if (!page)
> +        return nullptr;
> +    return toAPI(&API::FrameInfo::create(*toImpl(frameRef), WebCore::SecurityOriginData { WebCore::SecurityOrigin::createFromString(toImpl(frameRef)->url())->data() }, *page).leakRef());
>  }

This creator being changed to return null is alarming. Does Safari still use this SPI? If so, they almost certainly don't null check the result?

[Alex Christensen]

Created attachment 383588 [details]
Patch

[Brady Eidson]

Comment on attachment 383588 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIFrameInfo.cpp:61
> +FrameInfo::~FrameInfo() = default;

We have to say this because of the virtual ness, but can't it be in the header?

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 383588 [details]:

imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html bug 202003 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 383588 [details]
Patch

Clearing flags on attachment: 383588

Committed r252492: <https://trac.webkit.org/changeset/252492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57234919>

[Ryosuke Niwa]

Looks like this broke http/tests/navigation/window-open-redirect-and-remove-opener.html. It's hitting a debug assertion:

https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fnavigation%2Fwindow-open-redirect-and-remove-opener.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fnavigation%2Fwindow-open-redirect-and-remove-opener.html

[Truitt Savell]

I was able to reproduce the failure by running the test on r252492 but not r252491. 

reproduced with command:
run-webkit-tests http/tests/navigation/window-open-redirect-and-remove-opener.html --iterations 500 -f --debug --exit-after-n-crashes-or-timeouts 1

[Alex Christensen]

I'm looking into it.

[Alex Christensen]

https://trac.webkit.org/changeset/252601/webkit fixes the test.