Bug 187050 - Remove quarantine for Webex plugin
Summary: Remove quarantine for Webex plugin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 187081
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-26 09:55 PDT by youenn fablet
Modified: 2018-07-03 20:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.78 KB, patch)
2018-06-26 09:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (4.83 KB, patch)
2018-06-26 17:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Using qtn_proc_init if qtn_proc_init_with_self fails (6.61 KB, patch)
2018-07-03 16:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-06-26 09:55:09 PDT
Remove quarantine for Webex plugin
Comment 1 youenn fablet 2018-06-26 09:58:57 PDT
Created attachment 343613 [details]
Patch
Comment 2 Brent Fulgham 2018-06-26 10:26:23 PDT
Comment on attachment 343613 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=187050

Radar, please! :-)
Comment 3 Radar WebKit Bug Importer 2018-06-26 10:27:08 PDT
<rdar://problem/41478189>
Comment 4 youenn fablet 2018-06-26 17:13:56 PDT
Created attachment 343662 [details]
Patch for landing
Comment 5 youenn fablet 2018-06-26 17:14:21 PDT
(In reply to Brent Fulgham from comment #2)
> Comment on attachment 343613 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343613&action=review
> 
> r=me
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=187050
> 
> Radar, please! :-)

Thanks for the review, added radar link.
Comment 6 WebKit Commit Bot 2018-06-26 17:31:50 PDT
Comment on attachment 343662 [details]
Patch for landing

Clearing flags on attachment: 343662

Committed r233232: <https://trac.webkit.org/changeset/233232>
Comment 7 WebKit Commit Bot 2018-06-26 17:31:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Commit Bot 2018-06-26 21:33:40 PDT
Re-opened since this is blocked by bug 187081
Comment 9 youenn fablet 2018-07-03 16:22:10 PDT
Created attachment 344237 [details]
Using qtn_proc_init if qtn_proc_init_with_self fails
Comment 10 EWS Watchlist 2018-07-03 16:23:22 PDT
Attachment 344237 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54:  qtn_proc_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Brent Fulgham 2018-07-03 16:52:49 PDT
Comment on attachment 344237 [details]
Using qtn_proc_init if qtn_proc_init_with_self fails

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

> Source/WebKit/PluginProcess/PluginProcess.h:78
> +    bool shouldOverrideQuarantine() final;

I feel like this would be better named "shouldEnableQuarantine()" rather than override. This really is the default case (we only "Don't Override" for WebEx).

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:697
> +    return m_pluginBundleIdentifier != "com.cisco.webex.plugin.gpc64";

Is there a 32-bit version we need to worry about?
Comment 12 youenn fablet 2018-07-03 20:03:59 PDT
(In reply to Brent Fulgham from comment #11)
> Comment on attachment 344237 [details]
> Using qtn_proc_init if qtn_proc_init_with_self fails
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344237&action=review
> 
> > Source/WebKit/PluginProcess/PluginProcess.h:78
> > +    bool shouldOverrideQuarantine() final;
> 
> I feel like this would be better named "shouldEnableQuarantine()" rather
> than override. This really is the default case (we only "Don't Override" for
> WebEx).

I was initially ok with the renaming but looking further in the code, overriding seems the right term.
There is a default value in Info.plist for WebContent/Network/Storage/Plugin processes and we sometimes override it.
Given the current Info.plist, we do not really need to 'override' the value fo WebContent/Network/Storage processes since LSFileQuarantineEnabled is already set to true for all of them.

> > Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:697
> > +    return m_pluginBundleIdentifier != "com.cisco.webex.plugin.gpc64";
> 
> Is there a 32-bit version we need to worry about?

I only encountered this one so we should be good there.
Comment 13 WebKit Commit Bot 2018-07-03 20:32:12 PDT
Comment on attachment 344237 [details]
Using qtn_proc_init if qtn_proc_init_with_self fails

Clearing flags on attachment: 344237

Committed r233497: <https://trac.webkit.org/changeset/233497>
Comment 14 WebKit Commit Bot 2018-07-03 20:32:14 PDT
All reviewed patches have been landed.  Closing bug.