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: rename WebSwapCGLLayer to WebSwapCGLLayerChromium #35961

Merged
merged 2 commits into from Jan 5, 2023

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Oct 10, 2022

Description of Change

Fixes #33685
This was being fixed in ANGLE https://chromium-review.googlesource.com/c/angle/angle/+/3062212
But now again both WebKit and Chromium have the duplicate symbol as they both use ANGLE

Checklist

Release Notes

Notes: Fixed warning about duplicate WebSwapCGLLayer symbols when Electron starts on macOS.

@miniak miniak added semver/patch backwards-compatible bug fixes target/19-x-y target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. labels Oct 10, 2022
@miniak miniak self-assigned this Oct 10, 2022
@miniak miniak requested a review from a team as a code owner October 10, 2022 11:15
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 10, 2022
@miniak miniak changed the title fix: rename to WebSwapCGLLayer to WebSwapCGLLayerChromium fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium Oct 10, 2022
@miniak miniak force-pushed the miniak/WebSwapCGLLayer branch 2 times, most recently from 1c1d1c1 to d8cec87 Compare October 10, 2022 11:29
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be submitted upstream

@miniak
Copy link
Contributor Author

miniak commented Oct 10, 2022

@nornagon How will it help if the new version of ANGLE makes it into WebKit in the new macOS? We will get the same error again.

@nornagon
Copy link
Member

Why are we linking against WebKit?

@miniak
Copy link
Contributor Author

miniak commented Oct 10, 2022

@nornagon I can check that, but it might be a transitive dependency of some other framework / library that we load

@miniak
Copy link
Contributor Author

miniak commented Oct 10, 2022

Electron is not directly linking WebKit

otool -L out/testing/Electron.app/Contents/Frameworks/Electron\ Framework.framework/Electron\ Framework 
out/testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Electron Framework:
	@rpath/Electron Framework.framework/Electron Framework (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1858.112.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/System/Library/Frameworks/IOSurface.framework/Versions/A/IOSurface (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/AVFoundation.framework/Versions/A/AVFoundation (compatibility version 1.0.0, current version 2.0.0)
	/System/Library/Frameworks/Carbon.framework/Versions/A/Carbon (compatibility version 2.0.0, current version 165.0.0)
	/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices (compatibility version 1.0.0, current version 56.0.0)
	/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics (compatibility version 64.0.0, current version 1557.5.4)
	/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1141.1.0)
	/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork (compatibility version 1.0.0, current version 1331.0.7)
	/System/Library/Frameworks/CoreText.framework/Versions/A/CoreText (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/LocalAuthentication.framework/Versions/A/LocalAuthentication (compatibility version 1.0.0, current version 985.100.54)
	/System/Library/Frameworks/QuartzCore.framework/Versions/A/QuartzCore (compatibility version 1.2.0, current version 1.11.0)
	/System/Library/Frameworks/CoreImage.framework/Versions/A/CoreImage (compatibility version 1.0.1, current version 5.0.0)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/Quartz.framework/Versions/A/Quartz (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/QuickLook.framework/Versions/A/QuickLook (compatibility version 1.0.0, current version 0.0.0)
	/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 60158.100.133)
	/System/Library/Frameworks/SecurityInterface.framework/Versions/A/SecurityInterface (compatibility version 1.0.0, current version 55159.100.4)
	/System/Library/Frameworks/ServiceManagement.framework/Versions/A/ServiceManagement (compatibility version 1.0.0, current version 2236.100.61)
	/System/Library/Frameworks/StoreKit.framework/Versions/A/StoreKit (compatibility version 1.0.0, current version 379.0.0)
	@rpath/Squirrel.framework/Squirrel (compatibility version 0.0.0, current version 0.0.0)
	@rpath/ReactiveObjC.framework/ReactiveObjC (compatibility version 0.0.0, current version 0.0.0)
	@rpath/Mantle.framework/Mantle (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 2113.40.126)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1858.112.0)
	/System/Library/Frameworks/IOKit.framework/Versions/A/IOKit (compatibility version 1.0.0, current version 275.0.0)
	/System/Library/Frameworks/OpenDirectory.framework/Versions/A/OpenDirectory (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration (compatibility version 1.0.0, current version 1163.100.19)
	/System/Library/Frameworks/CoreMedia.framework/Versions/A/CoreMedia (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Metal.framework/Versions/A/Metal (compatibility version 1.0.0, current version 261.13.0, weak)
	/System/Library/Frameworks/AudioToolbox.framework/Versions/A/AudioToolbox (compatibility version 1.0.0, current version 1000.0.0)
	/System/Library/Frameworks/AudioUnit.framework/Versions/A/AudioUnit (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreAudio.framework/Versions/A/CoreAudio (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa (compatibility version 1.0.0, current version 23.0.0)
	/System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
	/System/Library/Frameworks/VideoToolbox.framework/Versions/A/VideoToolbox (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/IOBluetooth.framework/Versions/A/IOBluetooth (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreBluetooth.framework/Versions/A/CoreBluetooth (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreMIDI.framework/Versions/A/CoreMIDI (compatibility version 1.0.0, current version 69.0.0)
	/System/Library/Frameworks/MediaAccessibility.framework/Versions/A/MediaAccessibility (compatibility version 1.0.0, current version 62.0.0)
	/System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/ForceFeedback.framework/Versions/A/ForceFeedback (compatibility version 1.0.0, current version 1.0.2)
	/System/Library/Frameworks/GameController.framework/Versions/A/GameController (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreWLAN.framework/Versions/A/CoreWLAN (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreLocation.framework/Versions/A/CoreLocation (compatibility version 1.0.0, current version 2667.0.23)
	/System/Library/Frameworks/ImageCaptureCore.framework/Versions/A/ImageCaptureCore (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/QuickLookThumbnailing.framework/Versions/A/QuickLookThumbnailing (compatibility version 1.0.0, current version 0.0.0, weak)
	/System/Library/Frameworks/MetalKit.framework/Versions/A/MetalKit (compatibility version 1.0.0, current version 151.0.0, weak)
	/System/Library/Frameworks/ScreenCaptureKit.framework/Versions/A/ScreenCaptureKit (compatibility version 1.0.0, current version 1.0.0, weak)
	/System/Library/Frameworks/MediaPlayer.framework/Versions/A/MediaPlayer (compatibility version 1.0.0, current version 1.0.0, weak)
	/System/Library/Frameworks/Vision.framework/Versions/A/Vision (compatibility version 1.0.0, current version 5.0.93, weak)
	@rpath/libffmpeg.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libcups.2.dylib (compatibility version 2.0.0, current version 2.14.0)
	/usr/lib/libbsm.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libpmenergy.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libpmsample.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libsandbox.1.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)

@miniak
Copy link
Contributor Author

miniak commented Oct 10, 2022

However when I set DYLD_PRINT_LIBRARIES and run Electron I can see libANGLE-shared.dylib from WebKit being loaded

...
dyld[92615]: <51E1BE1E-0E55-34EC-8ED1-4FC1CB9F4215> /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit
dyld[92615]: <A38D8109-544F-3335-B2EA-EC107A1D953C> /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebKitLegacy.framework/Versions/A/WebKitLegacy
dyld[92615]: <773783B1-CE89-30B0-8CE9-E4BC7C86EC21> /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/WebCore
dyld[92615]: <57920D53-CDF6-33F1-8517-0139388FFDEC> /System/Library/PrivateFrameworks/CorePrediction.framework/Versions/A/CorePrediction
dyld[92615]: <CAD05B86-F407-392F-A95C-53D98E1AC6DE> /usr/lib/libnetworkextension.dylib
dyld[92615]: <DCF979C2-7FC9-3D89-A00B-39799B548153> /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libwebrtc.dylib
dyld[92615]: <E5D41B65-5FC9-3DE2-94FC-537C9B20894F> /System/Library/PrivateFrameworks/SafariSafeBrowsing.framework/Versions/A/SafariSafeBrowsing
dyld[92615]: <A1FA96E9-A887-3981-A56E-3F8C6A906266> /System/Library/Frameworks/SecurityInterface.framework/Versions/A/SecurityInterface
dyld[92615]: <BCE43484-3B72-319C-9A88-4183B5870C3B> /System/Library/PrivateFrameworks/WebInspectorUI.framework/Versions/A/WebInspectorUI
dyld[92615]: <7A8C93B2-8A91-3F5B-A462-B3184DF8F0D3> /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libANGLE-shared.dylib
...

@miniak miniak requested a review from nornagon October 10, 2022 17:08
@nornagon
Copy link
Member

nornagon commented Oct 10, 2022

After quite a bit of fooling around, I determined that it's Quartz.framework which produces the dependency on WebKit. Here's the path:

Dependency path from Electron Framework to WebKit via Quartz, ImageKit, VisionKitCore, Reveal, Lookup, and DataDetectors

Chromium also links to Quartz.framework so presumably Chromium has the exact same issue.

@nornagon
Copy link
Member

Indeed:

$ /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary
objc[17564]: Class WebSwapCGLLayer is implemented in both /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libANGLE-shared.dylib (0x22b735b50) and /Applications/Google Chrome Canary.app/Contents/Frameworks/Google Chrome Framework.framework/Versions/108.0.5352.0/Libraries/libGLESv2.dylib (0x109d4bf38). One of the two will be used. Which one is undefined.

So I think this should be WONTFIX. It's a macOS bug.

@deepak1556
Copy link
Member

Upstream bug https://bugs.chromium.org/p/chromium/issues/detail?id=1217851, response from Apple is interesting https://bugs.chromium.org/p/chromium/issues/detail?id=1217851#c6. @nornagon maybe you want to comment about the dependency chain on that issue, I think it could help move their Apple bug report forward ?

@miniak
Copy link
Contributor Author

miniak commented Oct 11, 2022

@nornagon why would it be a macOS bug? It's just a feature of the Objective-C runtime, you can't have two classes with the same name in a single process. The fact that Quartz.framework happens to transitively depend on WebKit.framework is an implementation detail that we need to live with.

I really don't see a major issue having a simple patch, which fixes the warning and has basically a 0% risk of breaking something. Actually without the patch we are risking something breaking as it's undefined, which version of the WebSwapCGLLayer would be instantiated and the implementation is not guaranteed to be identical between different versions of WebKit and Chromium.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 11, 2022
@nornagon
Copy link
Member

nornagon commented Oct 11, 2022

@miniak patches are maintenance burden. If we land this patch, later on it will break and we will have to figure out why and fix it.

Given Chromium is in the exact same situation and doesn't seem to be particularly bothered by it, I'm not sure I want to take on that maintenance burden. There's an upstream bug for this. IMO either we fix this upstream—maybe by suffixing the WebSwapCGLLayer class name with a unique-per-build ID, e.g. using ANGLE_COMMIT_HASH?—or we let it be.

I guess it's arguably not a macOS bug but an Objective-C "bug" that you can't hide an Objective-C class from clients of a library.

@nornagon
Copy link
Member

https://bugs.chromium.org/p/chromium/issues/detail?id=1217851#c11 suggests a reasonable fix upstream.

@miniak
Copy link
Contributor Author

miniak commented Oct 11, 2022

@nornagon, I know that very well, we have plenty of custom patches in our fork. It's a PITA. However the warning is annoying, therefore I've already applied this patch to our fork and will remove it when there is a better solution from upstream.

@jkleinsc jkleinsc added the target/23-x-y PR should also be added to the "23-x-y" branch. label Nov 30, 2022
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed my mind. the warning is annoying, the patch is small, and chrome will fix this properly eventually.

@zcbenz
Copy link
Member

zcbenz commented Jan 5, 2023

The failing test is a flaky one.

@zcbenz zcbenz merged commit 42cda4a into main Jan 5, 2023
@zcbenz zcbenz deleted the miniak/WebSwapCGLLayer branch January 5, 2023 06:49
@release-clerk
Copy link

release-clerk bot commented Jan 5, 2023

Release Notes Persisted

Fixed warning about duplicate WebSwapCGLLayer symbols when Electron starts on macOS.

trop bot added a commit that referenced this pull request Jan 5, 2023
* fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium

* undo changes to patches/config.json

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

Co-authored-by: Milan Burda <milan.burda@gmail.com>
@trop
Copy link
Contributor

trop bot commented Jan 5, 2023

I have automatically backported this PR to "21-x-y", please check out #36798

@trop trop bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Jan 5, 2023
@trop
Copy link
Contributor

trop bot commented Jan 5, 2023

I have automatically backported this PR to "23-x-y", please check out #36799

@trop
Copy link
Contributor

trop bot commented Jan 5, 2023

I have automatically backported this PR to "22-x-y", please check out #36800

@trop trop bot added in-flight/23-x-y merged/23-x-y PR was merged to the "23-x-y" branch. merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. and removed target/23-x-y PR should also be added to the "23-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. in-flight/23-x-y labels Jan 5, 2023
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium

* undo changes to patches/config.json

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
gecko19 pushed a commit to brightsign/electron that referenced this pull request Feb 28, 2023
* fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium

* undo changes to patches/config.json

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
gecko19 pushed a commit to brightsign/electron that referenced this pull request Mar 15, 2023
* fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium

* undo changes to patches/config.json

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
gecko19 pushed a commit to brightsign/electron that referenced this pull request Mar 15, 2023
* fix: rename WebSwapCGLLayer to WebSwapCGLLayerChromium

* undo changes to patches/config.json

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Warning: Class WebSwapCGLLayer is implemented in both system library and electron distribution
5 participants