RTCDispatcher implemented in two libraries |
|||||||||||
Issue descriptionI got this when running my Chrome dev build today on Mac: objc[17499]: Class RTCDispatcher is implemented in both /Users/(...)/src/out/Release/./libcontent.dylib (0x10cf402b8) and /Users/(...)/src/out/Release/libchrome_dll.dylib (0x107190af8). One of the two will be used. Which one is undefined. I'm on commit 83d37523e3af93d816f70c81f37b5ac1ab7b66c5. Mirko: I saw you've made some build related changes recently. Can you have a look?
,
Dec 18 2017
Hi, yes let me take a look.
,
Dec 19 2017
Here is a small investigation: The implementation of RTCDispatcher lives in //third_party/webrtc/sdk:common_objc. From GN I can see that both //content:content and //chrome:chrome_dll depend on //third_party/webrtc/sdk:common_objc and the dependencies on //third_party/webrtc/modules/desktop_capture:desktop_capture were not introduced recently. > gn path out/Default //content:content //third_party/webrtc/sdk:common_objc //content:content --[public]--> //content/public/browser:browser_sources --[private]--> //third_party/webrtc/modules/desktop_capture:desktop_capture --[public]--> //third_party/webrtc/modules/desktop_capture:desktop_capture_objc --[private]--> //third_party/webrtc/sdk:common_objc > gn path out/Default //chrome:chrome_dll //third_party/webrtc/sdk:common_objc //chrome:chrome_dll --[private]--> //chrome:browser_dependencies --[public]--> //chrome/browser:browser --[private]--> //third_party/webrtc/modules/desktop_capture:desktop_capture --[public]--> //third_party/webrtc/modules/desktop_capture:desktop_capture_objc --[private]--> //third_party/webrtc/sdk:common_objc There is also a path between //chrome:chrome_dll and //content:content: > gn path out/Default //chrome:chrome_dll //content:content //chrome:chrome_dll --[private]--> //content/public/app:both --[public]--> //content:content The last two changes on //content/renderer/BUILD.gn should not cause this behaviour (https://chromium-review.googlesource.com/c/chromium/src/+/810968, https://chromium-review.googlesource.com/c/chromium/src/+/829139). Are we sure that the message appeared recently?
,
Dec 19 2017
I don't recall seeing it last week. The question is why this and no other classes have multiple implementations?
,
Dec 19 2017
Right, this is the right question... I was looking in another direction (my changes in the build graph) but looking more carefully at the //third_party/webrtc/sdk:common_objc target I see that it has been added as a dependency to //third_party/webrtc/modules/desktop_capture starting from https://webrtc-review.googlesource.com/c/src/+/30902 (landed on 12/12/2017). I think that it can be the culprit. I don't have a clean explanation regarding why we get the error only for RTCDispatcher though.
,
Dec 19 2017
,
Dec 20 2017
zijiehe@: Mirko mentioned you might have suggestions on this, any thoughts? Mirko and I are ooo over the holidays. CC Patrik FYI.
,
Jan 4 2018
,
Jan 4 2018
Sorry for the late response. Yes, the change https://webrtc-review.googlesource.com/c/src/+/30902 added sdk:common_objc as the dependency of webrtc/modules/desktop_capture target. Unfortunately I am not an expert of objective-c, but it looks like objc implementations are loaded into runtime with a totally different manner. (See stackoverflow @ https://goo.gl/2kKrJ9) Since the change to desktop_capture target required only scoped_cftyperef.h, moving this class out of sdk:common_objc and adding a new target (maybe sdk:scoped_ref) should be a short term solution. But I would suggest to find some objective-c experts to handle the problem in long-term. AFAICT, sdk:common_objc is a shared library intentionally used cross the webrtc project. Including it from any target in webrtc should be expected. So the question becomes, 1. Is desktop_capture the only target in webrtc which has been included in both chrome_dll and content? If yes, why do we need to? Should we add a component in chromium to isolate the desktop_capture implementation? (component is not supported in webrtc AFAIK.) 2. If desktop_capture is not or will not be the only target in webrtc which has been included in both chrome_dll and content, is there a way to workaround the objective-c dynamic linkage problem without introducing a standalone component anymore?
,
Jan 5 2018
+me and rsesek@ (ObjC-friendly folks) Re. #9, yes, it needs to be in a component instead of ending up in the same target twice. Here's an example of fixing this kind of thing in the past: d295e88072c54f74421d8d9ba3dff50d6d5c9a0d
,
Jan 8 2018
zijiehe@ can you please re-assign to whoever should fix this?
,
Jan 8 2018
Brave, would you mind to have a look at this issue, since I am out of Chromium now? You may need to create a component in component build, say content/public/browser/BUILD.gn:desktop_capture, within Chromium to convert webrtc/desktop_capture from a library into a component, and use it in both content and chrome_dll targets.
,
Jan 9 2018
,
Jan 11 2018
,
Jan 11 2018
why it's M65 ReleaseBlock? This problem is in M64 already(maybe M63 too?) and there seems being no practical affection. I can't work on this immediately (will do it ASAP though). So back to Pri-2 and remove blocker.
,
Jan 11 2018
This happens in debug mode / component build only. So I agree this should not be a release blocker.
,
Jan 11 2018
> why it's M65 ReleaseBlock? This problem is in M64 already(maybe M63 too?) and there seems being no practical affection. It's a release block because Chrome is spewing error messages to the console on the Mac. While this bug may have slipped through in M64, that doesn't make it any less important to fix ASAP. So those are the reasons for RBB M65. > This happens in debug mode / component build only. So I agree this should not be a release blocker. I use component build so I am seeing it a lot (especially while I'm working on fixing browser_tests). I agree with Pri-2 given that it's not happening in release builds.
,
Jan 18 2018
I'm working on this now. With some initial reading, it seems there is more working than gn file modification, since we're sort of handling a third-party static lib into a component. Need to export the desktop capture APIs for it, maybe some more refactoring working due to this.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/209c526abb00ec381252c178b54b03bb55828091 commit 209c526abb00ec381252c178b54b03bb55828091 Author: braveyao <braveyao@chromium.org> Date: Wed Jan 24 18:58:38 2018 [desktop capture] remove dependencies on webrtc/desktop_capture from /chrome On OSX there is a runtime waring with component building, see crbug.com/795754 . This is because both //content:content and //chrome:chrome_dll depend on webrtc/desktop_capture, which depends on an objc class included recently. One way to solve it is to make webrtc/desktop_capture a component, which means tons of work to export all the methods. After reading the related codes, it seems it's unnecessary for chrome/browser to depend on webrtc/desktop_capture directly, but through APIs and public dependency in content/public/browser:browser_sources. Bug: 795754 Change-Id: I47ee9263a8cce034b42d0b45f555bd9f6d397069 Reviewed-on: https://chromium-review.googlesource.com/876723 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Zijie He <zijiehe@chromium.org> Commit-Queue: Weiyong Yao <braveyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#531628} [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/chrome/browser/BUILD.gn [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/chrome/browser/extensions/BUILD.gn [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/chrome/test/BUILD.gn [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/content/browser/media/capture/desktop_capture_device.cc [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/content/public/browser/BUILD.gn [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/content/public/browser/desktop_capture.cc [modify] https://crrev.com/209c526abb00ec381252c178b54b03bb55828091/content/public/browser/desktop_capture.h
,
Jan 24 2018
I didn't make webrtc/desktop-capture into a component in chromium, but a equivalent, but much simpler, method as removing dependency on webrtc/desktop-capture from /chrome. Neither of them is robust solution for this situation. The best solution may be supporting component building in webrtc. We can recheck this when that is feasible. Close this for now. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by grunell@chromium.org
, Dec 18 2017