Bug 204199

Summary: Reduce structure copies when creating an API::FrameInfo
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, berto, cgarcia, commit-queue, ews-watchlist, gustavo, jiewen_tan, rniwa, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Alex Christensen
Reported 2019-11-14 12:20:24 PST
WKFrameInfo.webView should not return nil
Attachments
Patch (88.26 KB, patch)
2019-11-14 12:21 PST, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (97.31 KB, patch)
2019-11-14 12:37 PST, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (100.53 KB, patch)
2019-11-14 15:17 PST, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (101.34 KB, patch)
2019-11-14 15:26 PST, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (97.72 KB, patch)
2019-11-14 17:09 PST, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2019-11-14 12:21:26 PST
Created attachment 383568 [details] Patch
Alex Christensen
Comment 2 2019-11-14 12:37:24 PST
Created attachment 383569 [details] Patch
EWS Watchlist
Comment 3 2019-11-14 12:38:31 PST
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
Comment 4 2019-11-14 15:17:34 PST
Created attachment 383577 [details] Patch
Alex Christensen
Comment 5 2019-11-14 15:26:06 PST
Created attachment 383579 [details] Patch
Brady Eidson
Comment 6 2019-11-14 15:36:54 PST
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
Comment 7 2019-11-14 17:09:28 PST
Created attachment 383588 [details] Patch
Brady Eidson
Comment 8 2019-11-15 10:29:23 PST
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
Comment 9 2019-11-15 11:36:08 PST
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: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2019-11-15 11:37:03 PST
Comment on attachment 383588 [details] Patch Clearing flags on attachment: 383588 Committed r252492: <https://trac.webkit.org/changeset/252492>
WebKit Commit Bot
Comment 11 2019-11-15 11:37:05 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2019-11-15 11:38:21 PST
<rdar://problem/57234919>
Truitt Savell
Comment 14 2019-11-18 14:17:40 PST
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
Comment 15 2019-11-18 14:38:31 PST
I'm looking into it.
Alex Christensen
Comment 16 2019-11-18 15:31:47 PST
https://trac.webkit.org/changeset/252601/webkit fixes the test.
Note You need to log in before you can comment on or make changes to this bug.