New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 795754 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

RTCDispatcher implemented in two libraries

Project Member Reported by grunell@chromium.org, Dec 18 2017

Issue description

I 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?
 
Description: Show this description
Status: Started (was: Assigned)
Hi, yes let me take a look.
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?
I don't recall seeing it last week. The question is why this and no other classes have multiple implementations?
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.
Cc: mbonadei@chromium.org
Owner: grunell@chromium.org
Cc: phoglund@chromium.org zijiehe@chromium.org
zijiehe@: Mirko mentioned you might have suggestions on this, any thoughts?

Mirko and I are ooo over the holidays. CC Patrik FYI.

Comment 8 by hbos@chromium.org, Jan 4 2018

Cc: guidou@chromium.org hbos@chromium.org
 Issue 798398  has been merged into this issue.

Comment 9 by zijiehe@google.com, 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?

Comment 10 by sdy@chromium.org, Jan 5 2018

Cc: rsesek@chromium.org sdy@chromium.org
+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
Owner: zijiehe@chromium.org
zijiehe@ can you please re-assign to whoever should fix this?
Owner: braveyao@chromium.org
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.
Cc: shrike@chromium.org
Labels: -Pri-2 M-65 ReleaseBlock-Beta Pri-1
Labels: -Pri-1 -ReleaseBlock-Beta Pri-2
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.  
This happens in debug mode / component build only. So I agree this should not be a release blocker.
> 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.
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.
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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