Closed Bug 1179568 Opened 4 years ago Closed 4 years ago

NS_WARNING: Re-registering a CID?

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: dhylands, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
I was also running ubuntu 14.04 LTS.
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
Blocks: logspam
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?
Flags: needinfo?(daniel)
My plan is to just disable this on linux (or more specifically only enabled it for FxOS).
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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)?
Attachment #8640639 - Flags: review?(sworkman)
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.
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.
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?
Flags: needinfo?(daniel)
(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.
I'm on it.
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,)
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.
Attachment #8643615 - Flags: feedback?(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 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.
Attachment #8643615 - Flags: feedback?(erahm) → feedback+
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.
Attachment #8640639 - Attachment is obsolete: true
Attachment #8640639 - Flags: review?(sworkman)
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.
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.)
Attachment #8643615 - Attachment is obsolete: true
Attachment #8645581 - Flags: review?(roc)
Depends on: 1191253
There, I'll just make sure bug 1191253 sticks, then we can land this one too.
https://hg.mozilla.org/mozilla-central/rev/c69e64043b99
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1202756
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8658604 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/96220ec5fe2e
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.