Bug 224577 – [Cocoa] re-enable test case WKInspectorDelegate.InspectorConfiguration

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

<rdar://70505272>

[Blaze Burg]

The fix for this is.. involved. Patch coming.

[Blaze Burg]

Created attachment 426457 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 426457 [details]
Patch v1.0

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

r=me

This looks generally fine to me, but I'd recommend getting someone more familiar with CSP (both the concept and the related WebKit logic) to look at it as well.

BTW I was poking around some other CSP tests and I came across `LegacySchemeRegistry::registerURLSchemeAsBypassingContentSecurityPolicy`.  Would that also be a valid solution to this?  Or does the "Legacy" part suggest that maybe we don't want to use this 😅

Also, is it possible to write a layout/API test that confirms the CSP state in Web Inspector so that we're not opening up any new CSP "holes"?

> Source/WebKit/ChangeLog:34
> +        Drive-by, fix memory leak the right way.

Could you explain this more?

> Source/WebKit/ChangeLog:37
> +        (WebKit::WebInspectorUIProxy::frontendLoaded): Call the client.

ditto (:34)

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:51
> +    return _allowedURLSchemesForCSP.autorelease();

IIRC, `RetainPtr::autorelease` will `std::exchange` the underlying pointer.  Do we want that here?  If so, can you rename this to `releaseAllowedURLSchemesForCSP`?  If not, should this just be `.get()` instead?  Or maybe `[_allowedURLSchemesForCSP copy]`?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:61
> +    NSMutableSet<NSURL *> *result = [[NSMutableSet alloc] initWithCapacity:2];
> +    [result addObject:[NSURL URLWithString:WebKit::WebInspectorUIProxy::inspectorPageURL()]];
> +    [result addObject:[NSURL URLWithString:WebKit::WebInspectorUIProxy::inspectorTestPageURL()]];
> +    _mainResourceURLsForCSP = result;

Do we expect/allow these values to change?  If not, do we need to recreate/reassign `_mainResourceURLsForCSP` each time, or can we make a lazy getter that initializes it once?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:112
> +        NSMutableDictionary *headerFields = @{

Should we `adoptNS`?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:118
> +    RetainPtr<NSMutableSet<NSString *>> allowedURLSchemes = adoptNS([NSMutableSet setWithObjects:WKInspectorResourceScheme, @"ws", @"wss", nil]);

Are `ws` and `wss` actually used in the Web Inspector frontend?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:119
> +    for (auto pair : _configuration->_configuration->urlSchemeHandlers())

`auto&`

[Joseph Pecoraro]

Comment on attachment 426457 [details]
Patch v1.0

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:62
> +        _inspector->evaluateInFrontendForTesting(JavaScriptSnippetToOpenURLExternally(url));

Ideally the memory fix would happen as a separate commit.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:56
> +    _allowedURLSchemesForCSP = [allowedURLSchemes copy];

`_allowedURLSchemesForCSP` is a RetainPtr, so you're copying and retaining. This should probably be:

```
_allowedURLSchemesForCSP = adoptNS([allowedURLSchemes copy]);
```

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:61
>> +    _mainResourceURLsForCSP = result;
> 
> Do we expect/allow these values to change?  If not, do we need to recreate/reassign `_mainResourceURLsForCSP` each time, or can we make a lazy getter that initializes it once?

+1 for Devin's comment.

But again this is a RetainPtr so you're seeing an alloc + retain. This should probably be:

```
_mainResourceURLsForCSP = adoptNS([[NSMutableSet alloc] initWithCapacity:2]);
[_mainResourceURLsForCSP addObject:...];
[_mainResourceURLsForCSP addObject:...];
```

We prefer the `adoptNS` immediately on the line with copy/alloc.

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:112
>> +        NSMutableDictionary *headerFields = @{
> 
> Should we `adoptNS`?

+1. The `mutableCopy` would be leaked otherwise.

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:118
>> +    RetainPtr<NSMutableSet<NSString *>> allowedURLSchemes = adoptNS([NSMutableSet setWithObjects:WKInspectorResourceScheme, @"ws", @"wss", nil]);
> 
> Are `ws` and `wss` actually used in the Web Inspector frontend?

+1 to Devin's comment. Looks like the macOS one may not need it. It was added with:

    commit 835af68ab239f1577787fd759f67f08e8aad2f8d
    Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
    Date:   Thu Apr 14 17:44:20 2016 +0000

        REGRESSION: Web Inspector: Remote inspector doesn't work
        https://bugs.webkit.org/show_bug.cgi?id=156543
    
        Patch by Carlos Garcia Campos <[e-mail]> on 2016-04-14
        Reviewed by Timothy Hatcher.
    
        WebSocket connection is blocked by CSP, but needed by the remote web inspector to work, so allow connect to ws
        URLs from the web inspector. Also add stubs for zoomFactor and setZoomFactor to InspectorFrontendHostStub,
        required after r199396.

---

Also, this is one where we do not want to adopt, because it is already an auto released object.

Either:
```
RetainPtr<NSMutableSet<NSString *>> allowedURLSchemes = [NSMutableSet setWithObjects:WKInspectorResourceScheme, @"ws", @"wss", nil];
```

Preferred:
```
RetainPtr<NSMutableSet<NSString *>> allowedURLSchemes = adoptNS([[NSMutableSet alloc] initWithObjects:WKInspectorResourceScheme, @"ws", @"wss", nil]);
```

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:122
> +    [inspectorSchemeHandler setAllowedURLSchemesForCSP:allowedURLSchemes.autorelease()];

Not sure we would want to autorelease here. Just let the method copy/retain appropriately, then we don't touch autorelease pools at all.

[Darin Adler]

Comment on attachment 426457 [details]
Patch v1.0

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

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:122
>> +    [inspectorSchemeHandler setAllowedURLSchemesForCSP:allowedURLSchemes.autorelease()];
> 
> Not sure we would want to autorelease here. Just let the method copy/retain appropriately, then we don't touch autorelease pools at all.

The correct code for a case like this is to just use get(), not autorelease().

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:123
>      [configuration setURLSchemeHandler:inspectorSchemeHandler.autorelease() forURLScheme:WKInspectorResourceScheme];

Same here.

[Darin Adler]

Comment on attachment 426457 [details]
Patch v1.0

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

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:51
>> +    return _allowedURLSchemesForCSP.autorelease();
> 
> IIRC, `RetainPtr::autorelease` will `std::exchange` the underlying pointer.  Do we want that here?  If so, can you rename this to `releaseAllowedURLSchemesForCSP`?  If not, should this just be `.get()` instead?  Or maybe `[_allowedURLSchemesForCSP copy]`?

This should be one of these:

    return _allowedURLSchemesForCSP.get();
    return adoptNS([_allowedURLSchemesForCSP copy]).autorelease();

We don’t want to null out the pointer. Not sure we need autorelease.

[Blaze Burg]

Created attachment 426862 [details]
Patch v1.1

[EWS]

Committed r276476 (236935@main): <https://commits.webkit.org/236935@main>

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