Bug 222177

Summary: Review remaining usage of autorelease to make sure it is necessary
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, darin, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, keith_miller, mark.lam, msaboff, philipj, saam, sam, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 229056    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2021-02-19 09:58:55 PST
Review remaining usage of autorelease to make sure it is necessary.
Attachments
Patch (81.46 KB, patch)
2021-02-19 10:07 PST, Chris Dumez
no flags
Patch (80.60 KB, patch)
2021-02-19 11:57 PST, Chris Dumez
no flags
Patch (82.16 KB, patch)
2021-02-19 16:00 PST, Chris Dumez
no flags
Patch (84.14 KB, patch)
2021-02-19 21:01 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2021-02-19 10:07:00 PST
Chris Dumez
Comment 2 2021-02-19 11:57:13 PST
Chris Dumez
Comment 3 2021-02-19 16:00:12 PST
Geoffrey Garen
Comment 4 2021-02-19 20:24:25 PST
Comment on attachment 421053 [details] Patch Can't tell if the safe browsing API test is related or not.
Chris Dumez
Comment 5 2021-02-19 20:27:43 PST
(In reply to Geoffrey Garen from comment #4) > Comment on attachment 421053 [details] > Patch > > Can't tell if the safe browsing API test is related or not. I will check before landing. Thanks for reviewing.
Chris Dumez
Comment 6 2021-02-19 21:01:31 PST
EWS
Comment 7 2021-02-19 22:37:07 PST
Committed r273194: <https://commits.webkit.org/r273194> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421076 [details].
Radar WebKit Bug Importer
Comment 8 2021-02-19 22:38:14 PST
Darin Adler
Comment 9 2021-02-20 09:53:18 PST
Comment on attachment 421076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421076&action=review I spotted at least one serious problem here. > Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71 > + auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil; This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update. > Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:83 > + auto update = !shippingContactUpdate ? adoptNS([PAL::allocPKPaymentRequestShippingContactUpdateInstance() initWithErrors:@[] paymentSummaryItems:_summaryItems.get() shippingMethods:_shippingMethods.get()]) : nil; Same mistake. > Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:91 > + auto update = !shippingMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestShippingMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil; Same mistake.
Chris Dumez
Comment 10 2021-02-20 09:57:03 PST
Comment on attachment 421076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421076&action=review >> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71 >> + auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil; > > This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update. Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S Will fix.
Darin Adler
Comment 11 2021-02-20 10:08:17 PST
Comment on attachment 421076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421076&action=review >>> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71 >>> + auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil; >> >> This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update. > > Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S Will fix. For future reference, ?: is a gnu/clang extension and this a ?: b is a ? a : b Except it only evaluates "a" once. Since I learned perl first, I think of it as the same as the || operator in perl.
Chris Dumez
Comment 12 2021-02-20 10:14:25 PST
(In reply to Chris Dumez from comment #10) > Comment on attachment 421076 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421076&action=review > > >> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71 > >> + auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil; > > > > This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update. > > Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S > Will fix. Follow-up fix: <https://commits.webkit.org/r273201>. Thanks for spotting this.
Note You need to log in before you can comment on or make changes to this bug.