It should be possible for clients to suppress the availability of plug-ins on a per-plug-in/per-host basis.
<rdar://problem/19632424>
Created attachment 248348 [details] Patch
Comment on attachment 248348 [details] Patch Removing r? pending EWS analysis.
Created attachment 248351 [details] Patch
Comment on attachment 248351 [details] Patch Posted new patch trying to fix the Windows build.
Comment on attachment 248351 [details] Patch Attachment 248351 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4777809457184768 New failing tests: plugins/plugin-javascript-access.html plugins/navigator-mimeTypes-length.html
Created attachment 248359 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
I am looking at the test failures.
Created attachment 248459 [details] Patch
Comment on attachment 248459 [details] Patch Attachment 248459 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4800679084294144 New failing tests: compositing/backgrounds/background-image-with-negative-zindex.html
Created attachment 248477 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248459&action=review I think you also need to store the various load policies in WebProcessCreationParameters. Patch looks good overall, but please fix the comments I had and upload a new version. > Source/WebCore/plugins/PluginData.h:32 > +enum PluginLoadClientPolicy : uint8_t { No need for : uint8_t here. Can't this just be called PluginLoadPolicy? > Source/WebCore/plugins/PluginData.h:35 > + PluginLoadClientUndefined = 0, This should be prefixed PluginLoadPolicy, not PluginLoadClient. > Source/WebCore/plugins/PluginStrategy.h:43 > +#if PLATFORM(MAC) > + virtual void setPluginLoadClientPolicy(PluginLoadClientPolicy, const String& host, const String& bundleIdentifier, const String& versionString) = 0; > + virtual void clearPluginClientPolicies() = 0; > +#endif I don't think these two need to be in the base class. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:574 > + encoder << static_cast<uint8_t>(pluginInfo.clientLoadPolicy); Should use encodeEnum instead. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:597 > + uint8_t clientLoadPolicy; > + if (!decoder.decode(clientLoadPolicy)) > + return false; > + pluginInfo.clientLoadPolicy = static_cast<PluginLoadClientPolicy>(clientLoadPolicy); decodeEnum. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:302 > +#endif // PLATFORM(MAC) No need for a comment here. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:330 > +#endif // PLATFORM(MAC) No need for a comment. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:112 > + void populatePluginCache(const WebCore::Page*); Should be a reference, not a pointer. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:120 > + bool pluginLoadClientPolicyForHost(const String&, const WebCore::PluginInfo&, WebCore::PluginLoadClientPolicy&); I think this should be const.
Comment on attachment 248459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248459&action=review Thanks for the feedback; I'll post a new version. >> Source/WebCore/plugins/PluginData.h:32 >> +enum PluginLoadClientPolicy : uint8_t { > > No need for : uint8_t here. Can't this just be called PluginLoadPolicy? Code generation for Web Replay (using WebInputs.json) requires non-class-enclosed enums used therein to have a storage type, and setting a storage type there without setting it here causes a build failure, so it looks we explicitly type such enums (see, e.g., PlatformWheelEventPhase). I could probably move the enum into the class, but it seems simpler to just give it a type. We already have a PluginModuleLoadPolicy which means a different thing and which is routinely stored into variables or returned by functions called pluginLoadPolicy, so I deliberately wanted to distinguish this as a policy that is declaratively set by the client. >> Source/WebCore/plugins/PluginData.h:35 >> + PluginLoadClientUndefined = 0, > > This should be prefixed PluginLoadPolicy, not PluginLoadClient. I kept the "Client" terminology here for the same reason, especially since I think it would be useful to have parallel policies (e.g. a PluginLoadSandboxPolicy) in the future. To make this sound better, though, I changed to PluginLoadClientPolicy, and changed the corresponding WK2 API policies to match. >> Source/WebCore/plugins/PluginStrategy.h:43 >> +#endif > > I don't think these two need to be in the base class. I agree, but WebProcess.cpp appears to only know about members declared in the base class. Is there a workaround that I'm missing? >> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:574 >> + encoder << static_cast<uint8_t>(pluginInfo.clientLoadPolicy); > > Should use encodeEnum instead. Fixed. >> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:597 >> + pluginInfo.clientLoadPolicy = static_cast<PluginLoadClientPolicy>(clientLoadPolicy); > > decodeEnum. Fixed. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:302 >> +#endif // PLATFORM(MAC) > > No need for a comment here. Removed. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:330 >> +#endif // PLATFORM(MAC) > > No need for a comment. Removed. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:112 >> + void populatePluginCache(const WebCore::Page*); > > Should be a reference, not a pointer. This is a pointer since WebPlatformStrategies::getPluginInfo() already took a pointer. I changed to use a reference internally, though it feels a little awkward. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:120 >> + bool pluginLoadClientPolicyForHost(const String&, const WebCore::PluginInfo&, WebCore::PluginLoadClientPolicy&); > > I think this should be const. Fixed.
Created attachment 248591 [details] Patch
Comment on attachment 248591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248591&action=review > Source/WebKit2/WebProcess/WebProcess.cpp:395 > + for (auto hostIter = parameters.pluginLoadClientPolicies.begin(); hostIter != parameters.pluginLoadClientPolicies.end(); ++hostIter) { > + for (auto bundleIdentifierIter = hostIter->value.begin(); bundleIdentifierIter != hostIter->value.end(); ++bundleIdentifierIter) { > + for (auto versionIter = bundleIdentifierIter->value.begin(); versionIter != bundleIdentifierIter->value.end(); ++versionIter) > + platformStrategies()->pluginStrategy()->setPluginLoadClientPolicy(static_cast<PluginLoadClientPolicy>(versionIter->value), hostIter->key, bundleIdentifierIter->key, versionIter->key); > + } > + } > +#endif Can't you use range-based for loops here?
(In reply to comment #16) > Comment on attachment 248591 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248591&action=review > > > Source/WebKit2/WebProcess/WebProcess.cpp:395 > > + for (auto hostIter = parameters.pluginLoadClientPolicies.begin(); hostIter != parameters.pluginLoadClientPolicies.end(); ++hostIter) { > > + for (auto bundleIdentifierIter = hostIter->value.begin(); bundleIdentifierIter != hostIter->value.end(); ++bundleIdentifierIter) { > > + for (auto versionIter = bundleIdentifierIter->value.begin(); versionIter != bundleIdentifierIter->value.end(); ++versionIter) > > + platformStrategies()->pluginStrategy()->setPluginLoadClientPolicy(static_cast<PluginLoadClientPolicy>(versionIter->value), hostIter->key, bundleIdentifierIter->key, versionIter->key); > > + } > > + } > > +#endif > > Can't you use range-based for loops here? Yes! for (const auto& hostKeyValue : parameters.pluginLoadClientPolicies) { for (const auto& bundleIdentifierKeyValue : hostKeyValue.value) { for (const auto& versionStringKeyValue : bundleIdentifierKeyValue.value) platformStrategies()->pluginStrategy()->setPluginLoadClientPolicy(static_cast<PluginLoadClientPolicy>(versionStringKeyValue.value), hostKeyValue.key, bundleIdentifierKeyValue.key, versionStringKeyValue.key); } }
Committed r181483: <http://trac.webkit.org/changeset/181483>
This caused memory use-after-free on many tests, detected by ASan. Will roll out, and will e-mail Conrad with details.
Re-opened since this is blocked by bug 142688
There was a pointer to stack memory being returned in one function as a result of mechanical refactoring. I'm testing a fix.
Created attachment 248711 [details] Patch
Comment on attachment 248711 [details] Patch Clearing flags on attachment: 248711 Committed r181562: <http://trac.webkit.org/changeset/181562>
All reviewed patches have been landed. Closing bug.
Comment on attachment 248711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248711&action=review > Source/WebCore/loader/SubframeLoader.cpp:174 > + const MimeClassInfo& mimeClassInfo = mimes[i]; A little bit late but this seems the perfect use case for range-based for loops. for (const auto& mimeClassInfo : mimes) > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:292 > +#if PLATFORM(MAC) Why MAC specific?
(In reply to comment #25) > Comment on attachment 248711 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248711&action=review > > > Source/WebCore/loader/SubframeLoader.cpp:174 > > + const MimeClassInfo& mimeClassInfo = mimes[i]; > > A little bit late but this seems the perfect use case for range-based for > loops. > > for (const auto& mimeClassInfo : mimes) Yeah, I was just trying to keep things as close to what was already there as possible. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:292 > > +#if PLATFORM(MAC) > > Why MAC specific? No reason it has to be, but I didn't want to opt other platforms in by default.
Comment on attachment 248711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248711&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:335 > +bool WebPlatformStrategies::pluginLoadClientPolicyForHost(const String& host, const PluginInfo& info, PluginLoadClientPolicy& policy) const Why do not expose this method too? I mean, with the current methods (if I'm not wrong) you are able to set the policy for any host, you can clear the policies for all plugins and you can get the policy for the current host. But you cant get the policy for a arbitrary host or list the host which have an specific policy. The use case could be have a list of hosts with their policies and could modify them.