Bug 220525

<rdar://problem/68443565>

Created attachment 417410 [details] Patch

Comment on attachment 417410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417410&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:5024 > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt, managerHandle); What is “managerHandle”? Doesn’t look like this compiles.

Created attachment 417411 [details] Patch

(In reply to Darin Adler from comment #3) > Comment on attachment 417410 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417410&action=review > > > Source/WebKit/UIProcess/WebPageProxy.cpp:5024 > > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt, managerHandle); > > What is “managerHandle”? Doesn’t look like this compiles. Good catch! I have uploaded another patch. Thanks for reviewing!

Comment on attachment 417411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417411&action=review > Source/WebKit/ChangeLog:10 > + Currently, Network extension sandbox extensions are sent to the WebContent process as part of the load parameters, but this is too late in some cases. > + In these cases, the extensions can be sent along with the DidReceivePolicyDecision message. Given this I would have expected to see code *moving* but instead this patch has code *added*. > Source/WebKit/UIProcess/WebPageProxy.cpp:5027 > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500 > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt); > +#else > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager.content-filter"_s }, WTF::nullopt); > +#endif Would have liked to see an even tighter version of this that uses a local variable and has less code inside #if with "constexpr ASCIILiteral".

(In reply to Darin Adler from comment #6) > Comment on attachment 417411 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417411&action=review > > > Source/WebKit/ChangeLog:10 > > + Currently, Network extension sandbox extensions are sent to the WebContent process as part of the load parameters, but this is too late in some cases. > > + In these cases, the extensions can be sent along with the DidReceivePolicyDecision message. > > Given this I would have expected to see code *moving* but instead this patch > has code *added*. > Initially, I thought the extensions that are sent with the load parameters were needed, since the message with load parameters is normally sent before the policy decision message, but after reconsidering, I now think this can be removed, since the policy decision message should in all cases be handled in the WebContent process before the Network Extension services are being accessed. I will update the patch accordingly. > > Source/WebKit/UIProcess/WebPageProxy.cpp:5027 > > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500 > > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt); > > +#else > > + return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager.content-filter"_s }, WTF::nullopt); > > +#endif > > Would have liked to see an even tighter version of this that uses a local > variable and has less code inside #if with "constexpr ASCIILiteral". Will fix! Thanks for reviewing!

Created attachment 417412 [details] Patch

This patch has been verified to fix the issue.

Comment on attachment 417412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417412&action=review I think this is good, but would be improved by refactoring slightly. > Source/WebKit/UIProcess/WebPageProxy.cpp:302 > +#endif You won't need this Cocoa-specific include if you leave the implementation in WebPageProxyCocoa.mm and use a stub in WebPageProxy.cpp for non-Cocoa clients. > Source/WebKit/UIProcess/WebPageProxy.cpp:5018 > +static SandboxExtension::HandleArray createNetworkExtensionsSandboxExtensions(WebProcessProxy& process) I think this would be cleaner if this method stayed in WebPageProxyCocoa.mm, with a stub in WebPageProxy.cpp for non-Cocoa cases. > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:156 > Then you could just move the below code into a method body instead of deleting it here. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:346 > +#endif You could avoid adding this cocoa-specific header here, if you moved the Sandbox consumption to WebPageCocoa.mm, which already knows about content filtering and sandbox stuff. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3375 > +#endif I think this would be cleaner with a new method stub in this file called "consumeSandboxExtensions" that is a no-op for non-Cocoa builds. Then the real implementation could live in WebPageCocoa.mm, which already knows about all the important things and no new headers need to be added anywhere. > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:-90 > -#endif ... and this could just be moved to a new method (well, I guess the conditionals wouldn't be needed anymore in the new code path).

Created attachment 417554 [details] Patch

Created attachment 417556 [details] Patch

(In reply to Brent Fulgham from comment #10) > Comment on attachment 417412 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417412&action=review > > I think this is good, but would be improved by refactoring slightly. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:302 > > +#endif > > You won't need this Cocoa-specific include if you leave the implementation > in WebPageProxyCocoa.mm and use a stub in WebPageProxy.cpp for non-Cocoa > clients. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:5018 > > +static SandboxExtension::HandleArray createNetworkExtensionsSandboxExtensions(WebProcessProxy& process) > > I think this would be cleaner if this method stayed in WebPageProxyCocoa.mm, > with a stub in WebPageProxy.cpp for non-Cocoa cases. > > > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:156 > > > > Then you could just move the below code into a method body instead of > deleting it here. > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:346 > > +#endif > > You could avoid adding this cocoa-specific header here, if you moved the > Sandbox consumption to WebPageCocoa.mm, which already knows about content > filtering and sandbox stuff. > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3375 > > +#endif > > I think this would be cleaner with a new method stub in this file called > "consumeSandboxExtensions" that is a no-op for non-Cocoa builds. Then the > real implementation could live in WebPageCocoa.mm, which already knows about > all the important things and no new headers need to be added anywhere. > > > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:-90 > > -#endif > > ... and this could just be moved to a new method (well, I guess the > conditionals wouldn't be needed anymore in the new code path). These improvements have been addressed in the latest patch. Thanks for reviewing!

Created attachment 417557 [details] Patch

Comment on attachment 417557 [details] Patch Looks great! Thank you for addressing my earlier comments.

Comment on attachment 417557 [details] Patch Thanks for reviewing!

Committed r271469: <https://trac.webkit.org/changeset/271469> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417557 [details].