Bug 206911 – Web Inspector: safari app extension isolated worlds and injected files use the extension's identifier instead of its name

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/58026635>

[Devin Rousso]

Created attachment 389081 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Blaze Burg]

Comment on attachment 389081 [details]
Patch

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

Overall some great work. r- for bots. And I'd like InspectorClient to split into InspectorDelegate + InspectorDelegate::InspectorClient. (Unless you can get Brady or Alex to sign off on the correctness of the approach in this patch.)

> Source/JavaScriptCore/ChangeLog:3
> +        Web Inspector: safari app extension isolated worlds and injected files use the extension's identifier instead of it's name

Nit: its name

> Source/JavaScriptCore/ChangeLog:12
> +        debuggable rather than one per taret) and as such is not routed through the `Target` agent.

Nit: target

> Source/WebInspectorUI/ChangeLog:33
> +        should only be one per debuggable rather than one per taret) and as such is not routed

Nit: target

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:41
> +        // COMPATIBILITY (iOS 13.1): Browser did not exist yet.

Nit: 13.4

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:63
> +            // COMPATIBILITY (iOS 13.1): Browser did not exist yet.

Nit: 13.4

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:75
> +        return scheme && scheme.endsWith("-extension");

This seems kind of arbitrary, can the supported extension scheme syntax be documented in the ChangeLog or something?

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:111
> +            console.assert(!this._extensionNameIdentifierMap.has(extensionId), `Browser already exists with id ${extensionId}.`);

Nit: `Extension already exists..`

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:121
> +            console.assert(name);

Nit: needs assertion message, please include the extensionId as well.

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:36
> +        console.assert(Array.shallowEqual(Object.keys(this._agents), ["Browser", "Target"]));

Hah, nice! The design finally happened :P

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1552
> +    return static_cast<WebKit::InspectorClient&>(_page->inspectorClient()).delegate().autorelease();

Hmm, this seems a little weird to rely on WebPageProxy to keep a reference to the ObjC delegate. An approach like ResourceLoadDelegate / ResourceLoadClient would be easier to keep track of, IMO. The code would look more like:

- (id <_WKResourceLoadDelegate>)_resourceLoadDelegate
{
    return _resourceLoadDelegate->delegate().autorelease();
}

- (void)_setResourceLoadDelegate:(id<_WKResourceLoadDelegate>)delegate
{
    _page->setResourceLoadClient(_resourceLoadDelegate->createResourceLoadClient());
    _resourceLoadDelegate->setDelegate(delegate);
}

You can see the commit here: 
https://bugs.webkit.org/attachment.cgi?id=387046&action=review

This same pattern is followed for some other ObjC WKWebView delegates that plug into WebPageProxy.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:156
> +- (void)_browserExtensionsEnabled:(NSDictionary<NSString *, NSString *> *)extensionIDToName

Nit: extensionIDToNameMap

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:159
> +    transformed.reserveInitialCapacity([extensionIDToName count]);

Nit: extensionIDToName.count

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:171
> +        transformed.addVoid(extensionID);

addVoid? 👾

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorPrivate.h:35
> +- (void)_browserExtensionsEnabled:(NSDictionary<NSString *, NSString *> *)extensionIDToName;

I find it a bit strange to avoid using NSUUID here, but it seems pretty common across WebKit code to force NSUUID to NSString.

> Source/WebKit/UIProcess/Inspector/Agents/InspectorBrowserAgent.h:62
> +} // namespace WebCore

Nit: namespace WebKit

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorClient.h:64
> +} // WebKit

Nit: namespace WebKit

> Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp:510
> +{

I'm not quite sure how this test can fail if reserveInitialCapacity was incorrect. Is the initial capacity observable from calling .add, .remove, and .size a lot?

[Devin Rousso]

Comment on attachment 389081 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1552
>> +    return static_cast<WebKit::InspectorClient&>(_page->inspectorClient()).delegate().autorelease();
> 
> Hmm, this seems a little weird to rely on WebPageProxy to keep a reference to the ObjC delegate. An approach like ResourceLoadDelegate / ResourceLoadClient would be easier to keep track of, IMO. The code would look more like:
> 
> - (id <_WKResourceLoadDelegate>)_resourceLoadDelegate
> {
>     return _resourceLoadDelegate->delegate().autorelease();
> }
> 
> - (void)_setResourceLoadDelegate:(id<_WKResourceLoadDelegate>)delegate
> {
>     _page->setResourceLoadClient(_resourceLoadDelegate->createResourceLoadClient());
>     _resourceLoadDelegate->setDelegate(delegate);
> }
> 
> You can see the commit here: 
> https://bugs.webkit.org/attachment.cgi?id=387046&action=review
> 
> This same pattern is followed for some other ObjC WKWebView delegates that plug into WebPageProxy.

I was following what `DiagnosticLoggingClient` and `FindClient` did.  I have no preference one way or the other though, so I can change this.  It does seem slightly less efficient as now there is an object both on `WKWebView` and `WebPageProxy`, whereas the approach I have now only involves `WebPageProxy`.

>> Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp:510
>> +{
> 
> I'm not quite sure how this test can fail if reserveInitialCapacity was incorrect. Is the initial capacity observable from calling .add, .remove, and .size a lot?

...that's a good point!  I adopted this from the test for `HashMap`, which doesn't actually test the capacity either.

We could test `HashSet::capacity` directly, but I don't think that would return exactly `9999` as `HashTable::computeBestTableSize` rounds up to the nearest power of two, and then does some other math to ensure that there is still enough room.  I'll do the math and figure out what the expected capacity would be given `9999` and test for that too :)

[Devin Rousso]

Created attachment 389197 [details]
Patch

[Blaze Burg]

Comment on attachment 389197 [details]
Patch

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

r=me! (with EWS)

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:-241
> -/*! @abstract Called when a Web Inspector instance is attached to this WKWebView. This is not called in the case of remote inspection.

Please save my comment :)

[Devin Rousso]

Created attachment 389211 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389211 [details]
Patch

Rejecting attachment 389211 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
E_STALE_WHILE_REVALIDATE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_EVENTS -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESIZE_OBSERVER -DENABLE_RESOURCE_LOAD_STATISTICS -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SANDBOX_EXTENSIONS -DENABLE_SERVER_PRECONNECT -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SHAREABLE_RESOURCE -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USERSELECT_ALL -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_VARIATION_FONTS -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEBDRIVER_MOUSE_INTERACTIONS -DENABLE_WEBDRIVER_KEYBOARD_INTERACTIONS -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_CRYPTO -DENABLE_WEB_RTC -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DHAVE_CORE_PREDICTION -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DFRAMEWORK_NAME=WebKit -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.14 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/Source/WebKit -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources-normal/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat-security -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebKit2Prefix-aygnhujojvsdqwfdvhqwmhpyzahi/WebKit2Prefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource41.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource41.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource41.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource41.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource40-mm.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource40-mm.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: https://webkit-queues.webkit.org/results/13328890

[Devin Rousso]

Created attachment 391719 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391719 [details]
Patch

Clearing flags on attachment: 391719

Committed r257410: <https://trac.webkit.org/changeset/257410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.