1179568 - NS_WARNING: Re-registering a CID?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dave Hylands [:dhylands]]

I was recently debugging something (in B2G-desktop) and saw this warning:
https://dxr.mozilla.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp?from=nsComponentManager.cpp#551

The duplicated CID seems to be this one:
https://dxr.mozilla.org/mozilla-central/source/toolkit/system/dbus/nsDBusModule.cpp#28

which is also being registered here:

https://dxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetModule.cpp#944-954

It also seems dubious having 2 constants for the same CID:
https://dxr.mozilla.org/mozilla-central/source/toolkit/system/dbus/nsDBusModule.cpp#15-20
and
https://dxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetCID.h#958-963

I have no idea if this is actually causing any problems. I just saw the warning.

Comment 1

[Eric Rahm [:erahm]]

I've seen this locally as well (specifically on Ubuntu 14.04 LTS). A quick scan of a recent linux64 debug test runs doesn't show any instances, so it might be specific to the platform.

Comment 2

[Dave Hylands [:dhylands]]

I was also running ubuntu 14.04 LTS.

Comment 3

[Eric Rahm [:erahm]]

It turns out this *is* showing up during linux64 test runs, most often during shutdown.

I enabled logging to check which module it is during a try run and confirmed it's dbus related:

> While registering XPCOM module file:///builds/slave/test/build/application/firefox/components/libdbusservice.so, trying to re-register CID '{75a500a2-0030-40f7-86f8-63f225b940ae}' already registered by <static module>

Full warning stats:

> 674 [NNNNN] WARNING: Re-registering a CID?: file xpcom/components/nsComponentManager.cpp, line 551

This warning [1] shows up in the following test suites:

> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm123-tests1-linux64-build3.txt:100
> mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm121-tests1-linux64-build19.txt:82
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm68-tests1-linux64-build18.txt:78
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm123-tests1-linux64-build5.txt:74
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm123-tests1-linux64-build13.txt:57
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm115-tests1-linux64-build15.txt:50
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm114-tests1-linux64-build13.txt:49
> mozilla-central_ubuntu64_vm-debug_test-cppunit-bm122-tests1-linux64-build4.txt:40
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm121-tests1-linux64-build25.txt:27
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm113-tests1-linux64-build11.txt:16
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm113-tests1-linux64-build22.txt:16
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm115-tests1-linux64-build15.txt:14
> mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm54-tests1-linux64-build19.txt:14
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm121-tests1-linux64-build12.txt:12
> mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm51-tests1-linux64-build2.txt:8
> mozilla-central_ubuntu64_vm-debug_test-mochitest-jetpack-bm116-tests1-linux64-build2.txt:4
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm114-tests1-linux64-build6.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm116-tests1-linux64-build20.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm121-tests1-linux64-build15.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm116-tests1-linux64-build14.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm114-tests1-linux64-build8.txt:3
> mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm121-tests1-linux64-build7.txt:2
> mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm121-tests1-linux64-build12.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-push-bm51-tests1-linux64-build20.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-gl-bm67-tests1-linux64-build10.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm54-tests1-linux64-build11.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm122-tests1-linux64-build5.txt:2
> mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm114-tests1-linux64-build16.txt:1
> mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm113-tests1-linux64-build2.txt:1
> mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm114-tests1-linux64-build12.txt:1
> mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm68-tests1-linux64-build10.txt:1
> mozilla-central_ubuntu64_vm-debug_test-jsreftest-bm117-tests1-linux64-build7.txt:1
> mozilla-central_ubuntu64_vm-debug_test-crashtest-bm115-tests1-linux64-build2.txt:1

It shows up in 299 tests. A few of the most prevalent:

> 300 - Shutdown
> 8 - browser/components/sessionstore/test/browser_crashedTabs.js
> 6 - browser/base/content/test/social/browser_addons.js
> 5 - widget/tests/test_plugin_scroll_invalidation.html
> 5 - netwerk/test/browser/browser_child_resource.js
> 4 - toolkit/mozapps/extensions/test/xpinstall/browser_cookies4.js
> 4 - toolkit/mozapps/extensions/test/mochitest/test_bug687194.html
> 3 - toolkit/components/aboutmemory/tests/test_memoryReporters2.xul
> 3 - toolkit/components/aboutmemory/tests/test_dumpGCAndCCLogsToFile.xul
> 3 - toolkit/components/aboutmemory/tests/test_aboutmemory5.xul

[1] https://hg.mozilla.org/mozilla-central/annotate/9bca608ab65a/xpcom/components/nsComponentManager.cpp#l551

Comment 4

[Eric Rahm [:erahm]]

A somewhat interesting history here:

- NS_NETWORK_LINK_SERVICE_CID first showed up May 2006 in a windows only "detect online/offline status" patch (bug 76111) in netwerk
- :roc adds a linux only dbus version w/ the same CID (named NS_DBUS_NETWORK_LINK_SERVICE_CID)  in Dec 2006 (bug 312793) in toolkit
- ... other bits of history ...
- :bagder adds another version of NS_NETWORK_LINK_SERVICE_CID for linux in Jan 2015 in netwerk (bug 1008091), this version works for FxOS as well

So I guess bug 1008091 is to blame here. Do we just want to retire the dbus version? Should :bagder's version be FxOS only? Should the dbus version have been sharing the CID at all?

Daniel, any thoughts on what we should do here?

Comment 5

[Eric Rahm [:erahm]]

My plan is to just disable this on linux (or more specifically only enabled it for FxOS).

Comment 6

[Eric Rahm [:erahm]]

Attachment: Revert back to using DBUS network link service on linux

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8640639 [details] [diff] [review]
Revert back to using DBUS network link service on linux

This limits the use of nsNotifyAddrListener to just FxOS.

On linux, DBUS (the original owner of the NS_NETWORK_LINK_SERVICE_CID for linux) will be preserved. This keeps us from registering the same CID multiple times (with the first to register winning).

Steve it looks like you were one of the main reviewers in bug 1008091, would you mind taking a look at this (comment 4 provides some relevant history)?

Comment 8

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08fdc773dcf9

Comment 9

[Eric Rahm [:erahm]]

The try run confirms that all instances of "WARNING: Re-registering a CID?" have been removed.

Ideally we should make that warning an assertion as a follow-up so as to avoid this issue in the future.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Eric - thanks for looking into this. I would prefer to wait for Daniel's response to this. I think this will stop Gecko from being able to detect changes in network on Linux, and I don't think we want to do that.

Comment 11

[Daniel Stenberg [:bagder]]

Hm, what does a build need to get that dbus thing used? I can't recall having seen this problem when I worked on that code.

Steve is correct here. Removing the code for the desktop build might hamper Firefox's ability to detect network changes and act on the the way we like. The old dbus code isn't adjusted to the "network changed" logic.

I wonder if the dbus code actually offers anything the new code doesn't do?

Comment 12

[Eric Rahm [:erahm]]

(In reply to Daniel Stenberg [:bagder] from comment #11)
> Hm, what does a build need to get that dbus thing used? I can't recall
> having seen this problem when I worked on that code.

The warning will only show up in debug builds. Currently it's 100% reproducible on Ubuntu.

> Steve is correct here. Removing the code for the desktop build might hamper
> Firefox's ability to detect network changes and act on the the way we like.
> The old dbus code isn't adjusted to the "network changed" logic.
> 
> I wonder if the dbus code actually offers anything the new code doesn't do?

Would you mind taking a look at that? I'm fine with deprecating the dbus code (or just deleting it), but we need to choose one or the other.

Comment 13

[Daniel Stenberg [:bagder]]

I'm on it.

Comment 14

[Daniel Stenberg [:bagder]]

I'm about to submit a patch that removes toolkit/system/dbus completely, I find no reason to keep it as the new logic in netwerk/system/linux/ is supposed to cover that functionality (and more) and it works without dbus.

(While testing out this removal, I fell over bug 1191253 and bug 1191258 that also obstructed this online-offline functionality from working properly on Linux,)

Comment 15

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1179568-remove-toolkit-system-dbus-r-erahm.patch

When looking at who would be a suitable reviewer for this, I noticed the change history (for Bug 667980) "Expose network connection type to chrome." ... it certainly indicates that I cannot remove the whole thing like this...

So yeah, no try-run done yet, only verified on a local Linux build.

Comment 16

[Eric Rahm [:erahm]]

(In reply to Daniel Stenberg [:bagder] from comment #15)
> Created attachment 8643615 [details] [diff] [review]
> 0001-bug-1179568-remove-toolkit-system-dbus-r-erahm.patch
> 
> When looking at who would be a suitable reviewer for this, I noticed the
> change history (for Bug 667980) "Expose network connection type to chrome."
> ... it certainly indicates that I cannot remove the whole thing like this...

That feature was for android, for the linux/dbus version it just added a stub that sets the connection type to unknown (which you already do in the new linux version), so I don't think that's going to be an issue.

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 8643615 [details] [diff] [review]
0001-bug-1179568-remove-toolkit-system-dbus-r-erahm.patch

Review of attachment 8643615 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like the right thing to do once the bug 1191253 and bug 1191258 get landed. roc wrote the original code, he might be the one to give the final say so.

Comment 18

[Eric Rahm [:erahm]]

Comment on attachment 8640639 [details] [diff] [review]
Revert back to using DBUS network link service on linux

It looks like the new Linux version covers everything we need, lets go with Daniel's patch that removes the dbus version.

Comment 19

[Daniel Stenberg [:bagder]]

Great, thanks. The patch needs some further work though as it failed on try. I'll work on an update that behaves and ping roc about it.

Comment 20

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1179568-remove-toolkit-system-dbus-r-roc.patch

This patch version also removes the dbusservice from the install manifest and thus it works better on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fee88deec57

(The attached version was squashed and rebased after the try run.)

Comment 21

[Daniel Stenberg [:bagder]]

There, I'll just make sure bug 1191253 sticks, then we can land this one too.

Comment 22

[Daniel Stenberg [:bagder]]

Attachment: v3-0001-bug-1179568-remove-toolkit-system-dbus-r-roc.patch

Rebased.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69e64043b99

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c69e64043b99

Comment 25

[Stefan Sitter [:ssitter]]

Should it be removed from the other installer manifests too?
https://mxr.mozilla.org/mozilla-central/search?string=dbusservice&find=package-manifest.in

Comment 26

[Daniel Stenberg [:bagder]]

Oops, yes it should. I'll make a follow-up patch. It's a bit strange to me that no tests on try or elsewhere pointed this out...

Comment 27

[Daniel Stenberg [:bagder]]

Attachment: 0001-Bug-1179568-remove-dbusservice-from-more-installatio.patch

This is a additional change that removes "dbusservice" from a few remaining installation manifests that I didn't properly handle completely in the previous patch.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96220ec5fe2e

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96220ec5fe2e