Created attachment 367377 [details] Patch

Comment on attachment 367377 [details] Patch Assuming there are no thread safety issues and that LegacyCustomProtocolManagerProxy is always called from the same (main?) thread, r=me.

Comment on attachment 367377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367377&action=review Kinda wish we still canceled the load when the weak pointer became null. > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:-77 > - ASSERT(_customProtocolManagerProxy); I think this assertion might still be good. The WeakPtr can protect us from mistakes, but we should always be canceling before destroying the proxy, for efficiency reasons if no other reasons. > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:-78 > - _customProtocolManagerProxy = nullptr; Still might be good to null this out. Not sure. > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:119 > - if (redirectResponse) { > + if (_customProtocolManagerProxy && redirectResponse) { > _customProtocolManagerProxy->wasRedirectedToRequest(_customProtocolID, request, redirectResponse); > return nil; > } Seems to me that if _customProtocolManagerProxy is null we should return nil, not request.

(In reply to Anders Carlsson from comment #4) > Comment on attachment 367377 [details] > Patch > > Assuming there are no thread safety issues and that > LegacyCustomProtocolManagerProxy is always called from the same (main?) > thread, r=me. LegacyCustomProtocolManagerProxy is always called from the main thread. Thanks for reviewing!

(In reply to Darin Adler from comment #5) > Comment on attachment 367377 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367377&action=review > > Kinda wish we still canceled the load when the weak pointer became null. We should still cancel, because LegacyCustomProtocolManagerClient::invalidate() will still be called when the LegacyCustomProtocolManagerProxy is destroyed. > > > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:-77 > > - ASSERT(_customProtocolManagerProxy); > > I think this assertion might still be good. The WeakPtr can protect us from > mistakes, but we should always be canceling before destroying the proxy, for > efficiency reasons if no other reasons. > > > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:-78 > > - _customProtocolManagerProxy = nullptr; > > Still might be good to null this out. Not sure. I restored the assertion and the nulling. > > > Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm:119 > > - if (redirectResponse) { > > + if (_customProtocolManagerProxy && redirectResponse) { > > _customProtocolManagerProxy->wasRedirectedToRequest(_customProtocolID, request, redirectResponse); > > return nil; > > } > > Seems to me that if _customProtocolManagerProxy is null we should return > nil, not request. Yes, that makes more sense. Thanks for your comments!

Created attachment 367391 [details] Patch

The api-mac failure is due to a flaky API test. I filed <https://bugs.webkit.org/show_bug.cgi?id=196905>.

Comment on attachment 367391 [details] Patch Clearing flags on attachment: 367391 Committed r244246: <https://trac.webkit.org/changeset/244246>

All reviewed patches have been landed. Closing bug.