Bug 220394 - Use smart pointers in UIDelegate and NavigationState
Summary: Use smart pointers in UIDelegate and NavigationState
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-06 17:47 PST by Alex Christensen
Modified: 2021-01-07 15:10 PST (History)
3 users (show)

See Also:

Attachments
Patch (1.69 KB, patch)
2021-01-06 17:49 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (151.48 KB, patch)
2021-01-07 10:38 PST, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (151.67 KB, patch)
2021-01-07 10:53 PST, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (157.76 KB, patch)
2021-01-07 11:25 PST, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (149.43 KB, patch)
2021-01-07 12:18 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (151.37 KB, patch)
2021-01-07 13:53 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-01-06 17:47:55 PST 
Protect WebPageProxy in didFinishLoadForFrame
Comment 1 Alex Christensen 2021-01-06 17:49:22 PST 
Created attachment 417144 [details]
Patch
Comment 2 Alex Christensen 2021-01-06 17:49:25 PST 
<rdar://problem/72496374>
Comment 3 Chris Dumez 2021-01-07 08:31:45 PST 
Comment on attachment 417144 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4770
> +    auto protectedThis = makeRef(*this);

This is unnecessary in functions called via IPC. We solved it at a global level years ago. If you look at WebPageProxy::didReceiveMessage() in generated code, you will see:
auto protectedThis = makeRef(*this);

We protect the WebPageProxy while calling every function due to an IPC message.
Comment 4 Alex Christensen 2021-01-07 10:38:37 PST 
Created attachment 417187 [details]
Patch
Comment 5 Alex Christensen 2021-01-07 10:53:05 PST 
Created attachment 417189 [details]
Patch
Comment 6 Alex Christensen 2021-01-07 11:25:07 PST 
Created attachment 417192 [details]
Patch
Comment 7 Alex Christensen 2021-01-07 12:18:01 PST 
Created attachment 417198 [details]
Patch
Comment 8 Geoffrey Garen 2021-01-07 12:50:07 PST 
Comment on attachment 417198 [details]
Patch

r=me

Smart pointer is definitely better than raw pointer here. Would be nice in the future just to make this lifetime mismatch impossible.
Comment 9 Alex Christensen 2021-01-07 13:53:40 PST 
Comment on attachment 417198 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:408
> -        return;
> +        return completionHandler(false);

Surprisingly this changed behavior in an undesirable way.  Will revert.
Comment 10 Alex Christensen 2021-01-07 13:53:52 PST 
Created attachment 417209 [details]
Patch
Comment 11 EWS 2021-01-07 15:10:55 PST 
Committed r271264: <https://trac.webkit.org/changeset/271264>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417209 [details].