Allow clients to selectively disable plug-ins

• Component: New Bugs
• Status: RESOLVED FIXED

[Conrad Shultz]

It should be possible for clients to suppress the availability of plug-ins on a per-plug-in/per-host basis.

[Conrad Shultz]

<rdar://problem/19632424>

[Conrad Shultz]

Created attachment 248348 [details]
Patch

[Conrad Shultz]

Comment on attachment 248348 [details]
Patch

Removing r? pending EWS analysis.

[Conrad Shultz]

Created attachment 248351 [details]
Patch

[Conrad Shultz]

Comment on attachment 248351 [details]
Patch

Posted new patch trying to fix the Windows build.

[Build Bot]

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

[Build Bot]

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

[Conrad Shultz]

I am looking at the test failures.

[Conrad Shultz]

Created attachment 248459 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Anders Carlsson]

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.

[Conrad Shultz]

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.

[Conrad Shultz]

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.

[Conrad Shultz]

Created attachment 248591 [details]
Patch

[Anders Carlsson]

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?

[Conrad Shultz]

(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);
        }
    }

[Conrad Shultz]

Committed r181483: <http://trac.webkit.org/changeset/181483>

[Alexey Proskuryakov]

This caused memory use-after-free on many tests, detected by ASan. Will roll out, and will e-mail Conrad with details.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142688

[Conrad Shultz]

There was a pointer to stack memory being returned in one function as a result of mechanical refactoring. I'm testing a fix.

[Conrad Shultz]

Created attachment 248711 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248711 [details]
Patch

Clearing flags on attachment: 248711

Committed r181562: <http://trac.webkit.org/changeset/181562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Villar Senin]

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?

[Conrad Shultz]

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

[Marcos Chavarría Teijeiro (irc: chavaone)]

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.