[Cocoa] WKCustomProtocolLoader should store a WeakPtr to its LegacyCustomProtocolManagerProxy

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Cocoa] WKCustomProtocolLoader should store a WeakPtr to its LegacyCustomProtocolManagerProxy

[Andy Estes]

rdar://problem/48318983

[Andy Estes]

Created attachment 367375 [details]
Patch

[Andy Estes]

Created attachment 367377 [details]
Patch

[Anders Carlsson]

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.

[Darin Adler]

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.

[Andy Estes]

(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!

[Andy Estes]

(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!

[Andy Estes]

Created attachment 367391 [details]
Patch

[Andy Estes]

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

[WebKit Commit Bot]

Comment on attachment 367391 [details]
Patch

Clearing flags on attachment: 367391

Committed r244246: <https://trac.webkit.org/changeset/244246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.