Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disable more private macOS APIs in MAS build #20965

Open
wants to merge 4 commits into
base: master
from

Conversation

@zcbenz
Copy link
Member

zcbenz commented Nov 5, 2019

Description of Change

Refs #20027.

This PR disables following private macOS APIs used in Chromium:

  • CAContext
  • CALayerHost

Chromium has 2 ways of rendering on macOS: one is using official IOSurface way, and one is CARemoteLayer API. The IOSurface way is used when software compositing is enabled, and when the remote layer API is not available.

This PR forces using IOSurface even when remote layer API is available, it essentially behaves the same with passing the --disable-remote-core-animation command line flag.

More information can be found at https://bugs.chromium.org/p/chromium/issues/detail?id=312462.

  • NSAccessibilityRemoteUIElement

All related code using this class are disabled. It would affect accessibility when remote layer API is used, but we should be fine since remote layer API is disabled too.

  • NSNextStepFrame
  • NSThemeFrame

Chromium overrides these private classes to implement custom frame. All related code are disabled so custom frame would not work.

Since in Electron we only support standard frame and simple frameless window, we are not affected.

  • NSURLFileTypeMappings

Chromium uses it to guess mime type from extension names, the iOS version of Chromium does not use it. The fallback detection should be enough for us.

Checklist

Release Notes

Notes: Fix Electron apps getting rejected to Mac App Store.

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Nov 5, 2019

The original issue should be kept open since this PR does not patch fileport_makefd and fileport_makeport, and I don't know if it is needed.

(I hope it is not needed since these two are really difficult to get rid of.)

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 5, 2019

@zcbenz has manually backported this PR to "8-x-y", please check out #20966

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 5, 2019

@zcbenz has manually backported this PR to "7-1-x", please check out #20967

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 5, 2019

@zcbenz has manually backported this PR to "6-1-x", please check out #20970

@trop trop bot added in-flight/6-1-x and removed target/6-1-x labels Nov 5, 2019
Copy link
Member

deepak1556 left a comment

Thanks @zcbenz !

The RemoteMacViews are gonna stay for a long time https://bugs.chromium.org/p/chromium/issues/detail?id=859152
https://bugs.chromium.org/p/chromium/issues/detail?id=958255

We should look into the possibility of pushing the remote implementation behind a buildflag in upstream to reduce our patch in future. Thoughts ?

]

+ if (is_mas_build) {
+ sources -= [

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Nov 5, 2019

Member

why are we excluding them here ?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Nov 5, 2019

Author Member

remote_accessibility_api.mm implements ui::RemoteAccessibility which uses private APIs, excluding it is simpler than commenting out the whole file.

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Nov 5, 2019

Member

But we also seem to be patching them below is that needed ?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Nov 5, 2019

Author Member

In theory it is fine leaving remote_accessibility_api.h untouched, but by commenting out the declarations defined in remote_accessibility_api.h, any other file that uses the private APIs would trigger compilation error early, so we won't miss things when updating/backporting the patch.

@deepak1556 deepak1556 removed the target/3-1-x label Nov 5, 2019
@deepak1556 deepak1556 removed the target/4-2-x label Nov 5, 2019
@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 5, 2019

3-1-x and 4-2-x are not supported release lines, hence removed their backport labels

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Nov 5, 2019

We should look into the possibility of pushing the remote implementation behind a buildflag in upstream to reduce our patch in future. Thoughts ?

It is definitely good idea, it would also keep Chromium maintaining the code path we use.

@deepak1556 deepak1556 requested a review from electron/wg-upgrades Nov 5, 2019
@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Nov 5, 2019

Switching to IOSurface will (according to that linked issue) result in a performance hit. Do we have any idea how to quantify that hit?

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Nov 6, 2019

Switching to IOSurface will (according to that linked issue) result in a performance hit. Do we have any idea how to quantify that hit?

I don't know how to measure the performance hit, they did not do any actual measurement neither in the issue.

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 6, 2019

@zcbenz has manually backported this PR to "5-0-x", please check out #20988

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 6, 2019

@cmgustavo has manually backported this PR to "4-2-x", please check out #21003

@trop trop bot added the in-flight/4-2-x label Nov 6, 2019
@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Nov 7, 2019

Status update: as disabling remote layer APIs might drag down performance significantly, we are currently waiting for responses from Apple to see if it is possible to unblock CAContext and CALayerHost for us, or whether is there a better way of patching to avoid performance hit.

@ngehlert

This comment has been minimized.

Copy link

ngehlert commented Nov 17, 2019

any new updates here?

@ncodeyx

This comment has been minimized.

Copy link

ncodeyx commented Nov 20, 2019

Also waiting for update. #20027 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.