Bug 224441 - Runtime disable plugins and replacements
Summary: Runtime disable plugins and replacements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on: 224449 224451 224453
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-12 10:16 PDT by Alex Christensen
Modified: 2021-10-27 12:57 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.40 KB, patch)
2021-04-12 10:20 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-04-12 10:16:22 PDT
Runtime disable plugins and replacements
Comment 1 Alex Christensen 2021-04-12 10:20:23 PDT
Created attachment 425763 [details]
Patch
Comment 2 Sam Weinig 2021-04-12 10:31:05 PDT
Disabling plugins seems like a good idea, but I am not totally clear on the motivation for disabling replacement. What is the goal with disabling replacement and does it have to be linked to this (e.g. do we only do replacement if plugins are enabled?)
Comment 3 Sam Weinig 2021-04-12 10:31:51 PDT
Comment on attachment 425763 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:903
>  - (void)setPlugInsEnabled:(BOOL)flag
>  {
> -    [self _setBoolValue: flag forKey: WebKitPluginsEnabledPreferenceKey];
>  }

This seems like a step beyond deprecation. This makes it so that apps that enable the feature won't work.
Comment 4 Alex Christensen 2021-04-12 10:43:59 PDT
(In reply to Sam Weinig from comment #2)
> Disabling plugins seems like a good idea, but I am not totally clear on the
> motivation for disabling replacement. What is the goal with disabling
> replacement and does it have to be linked to this (e.g. do we only do
> replacement if plugins are enabled?)
The replacement code is related to the existence of plugins.  We could do it in a separate patch, but we also want to do it right now so I didn't see a reason to.  It is old code that exists to be compatible with websites of more than a decade ago, which we believe have updated since Flash support has waned.

(In reply to Sam Weinig from comment #3)
> Comment on attachment 425763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425763&action=review
> 
> > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:903
> >  - (void)setPlugInsEnabled:(BOOL)flag
> >  {
> > -    [self _setBoolValue: flag forKey: WebKitPluginsEnabledPreferenceKey];
> >  }
> 
> This seems like a step beyond deprecation. This makes it so that apps that
> enable the feature won't work.

That is correct.  WebPreferences is already deprecated.  We will look at seed feedback, but we intend to remove all our NPAPI code if this goes smoothly.
Comment 5 Alex Christensen 2021-04-12 12:08:54 PDT
After some internal discussion, we have decided to proceed with this, but split it into three changes:
1. Changing WKWebView to be unable to load plug ins.
2. Changing WebView to be unable to load plug ins.
3. Disabling the plug in replacements.
Comment 6 Sam Weinig 2021-04-12 12:15:26 PDT
(In reply to Alex Christensen from comment #4)
> (In reply to Sam Weinig from comment #2)
> > Disabling plugins seems like a good idea, but I am not totally clear on the
> > motivation for disabling replacement. What is the goal with disabling
> > replacement and does it have to be linked to this (e.g. do we only do
> > replacement if plugins are enabled?)
> The replacement code is related to the existence of plugins.  We could do it
> in a separate patch, but we also want to do it right now so I didn't see a
> reason to.  It is old code that exists to be compatible with websites of
> more than a decade ago, which we believe have updated since Flash support
> has waned.
> 

Seems like it would be worth keeping around to me. Keeping old sites that use things like QuickTime seems like a worthy goal.

I think at the very least it should be in its own change so that it can be backed out separately.

> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 425763 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=425763&action=review
> > 
> > > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:903
> > >  - (void)setPlugInsEnabled:(BOOL)flag
> > >  {
> > > -    [self _setBoolValue: flag forKey: WebKitPluginsEnabledPreferenceKey];
> > >  }
> > 
> > This seems like a step beyond deprecation. This makes it so that apps that
> > enable the feature won't work.
> 
> That is correct.  WebPreferences is already deprecated.  We will look at
> seed feedback, but we intend to remove all our NPAPI code if this goes
> smoothly.

Then the title and description of this bug is very misleading. You are not just disabling plugins, you are removing support for them completely.

Given I have repeatedly asked about doing this and gotten the feedback that doing this requires getting data on usage, I am surprised to see this being done without that data. What has changed?
Comment 7 Alex Christensen 2021-04-12 12:40:36 PDT
Thank you for the feedback on the title of this patch.  I intend to retitle this after I finish getting the EWS data on which tests need updating.

If you have feedback on the decision process of why this is being done, please redirect that feedback to the internal thread.
Comment 8 Darin Adler 2021-04-12 15:28:24 PDT
Comment on attachment 425763 [details]
Patch

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

>>>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:903
>>>>  }
>>> 
>>> This seems like a step beyond deprecation. This makes it so that apps that enable the feature won't work.
>> 
>> That is correct.  WebPreferences is already deprecated.  We will look at seed feedback, but we intend to remove all our NPAPI code if this goes smoothly.
> 
> Then the title and description of this bug is very misleading. You are not just disabling plugins, you are removing support for them completely.
> 
> Given I have repeatedly asked about doing this and gotten the feedback that doing this requires getting data on usage, I am surprised to see this being done without that data. What has changed?

Sam’s question seems like a good one. Hadn’t heard that we reached the point where we can do this.

Not even a linked-on-or-after check?
Comment 9 Alex Christensen 2021-04-13 13:58:17 PDT
I looked at hundreds of important apps and none of them used NPAPI plugins.  The only references to NPAPI were from old embedded QTWebKit frameworks that have the ability to load plugins, but no plugins were found.

In many of the environments where it is possible that NPAPI is used for other plugins, our ability to gather metrics is also disabled.  This is our way to try to remove plugin support:  We will try the small changes in the bugs related to this bug and listen for feedback.  If we receive feedback that these are still used, it will be straightforward to revert them.  Otherwise, at some time in the future we will remove the code this disables and the associated tests.

One thing that has changed recently is the end of life of Flash has been reached.  The possibility of someone using Flash has prevented us from doing this in the past, but that reason is now gone.
Comment 10 Geoffrey Garen 2021-04-13 15:44:01 PDT
> Not even a linked-on-or-after check?

To address this question specifically: A linked-on-or-after check is a good way to prevent new uses of legacy technology while giving old uses time to update and age out. But our belief is that old uses have already aged out. So, we're discussing ways to find out for sure.
Comment 11 Radar WebKit Bug Importer 2021-04-19 10:17:14 PDT
<rdar://problem/76850750>