856777 - "ASSERTION: Using observer service after XPCOM shutdown" (mozilla::dom::Navigator::Invalidate)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

In a debug build:
1. Load the testcase
2. Click the button
3. Quit Firefox

Result:
###!!! ASSERTION: Using observer service after XPCOM shutdown!: 'Error', file xpcom/ds/nsObserverService.cpp, line 122

mozilla::dom::Navigator::Invalidate is calling nsObserverService::RemoveObserver with aTopic "plugin-info-updated".

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c90851414292
user:        David Keeler
date:        Thu Dec 20 16:53:21 2012 -0800
summary:     bug 820708 - refresh navigator.plugins when a plugin is enabled/disabled r=joshmoz

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Georg Fritzsche [:gfritzsche]]

David, can you take this follow-up?

Comment 3

[Benjamin Smedberg]

This would indicate that Navigator::Invalidate is never being called, at least for this particular instance...

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is quite broken.

The Navigator in the testcase is not Invalidated because it's created after FreeInnerObjects (which happens when window.close() is called).  Our willingness to create another Navigator after FreeInnerObjects is arguably a bug.

Because the observer service holds a strong reference to the Navigator, it's possible that shutting down the observer service will destroy the Navigator and trigger invalidation which will re-enter the observer service while it's shutting down.  If nothing else we need to make this a weak reference.

Once we add Navigator to WebIDL bindings this will leak the world until shutdown.  In which case Invalidate will be called well after the observer service has shut down.  That's probably ok though since we null check the observer service before using it in Invalidate.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

These changes really should have been reviewed by a DOM peer btw.

Comment 6

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

Ok - I understand how wrong the previous implementation was. This patch moves the plugin-info-updated observation to nsPluginArray. This way, nsPluginArray just "does the right thing" (i.e. updates itself appropriately) and doesn't have to be baby-sat by Navigator. The observer service still has a strong reference, but that's okay because we're not unregistering the observer in its destructor (which would never happen).

One question I have in this patch is if I handled the FromSupports implementation properly - now that nsPluginArray is also an nsIObserver, there's an ambiguous nsISupports base.

Another remaining issue is I wasn't able to use the crashtest framework to create an automated test for this. Any ideas?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

window.open doesn't work in crashtest.

Comment 8

[Dana Keeler (she/her) [:keeler]]

I got it window.open to work by setting pref(dom.disable_open_during_load,false) in crashtests.list, but the assertion never fired.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 732453 [details] [diff] [review]
patch

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

This works today, but it's not going to work once Bug 855611 lands, and not just because that removes nsPluginArray::Invalidate.  Once nsPluginArray is converted to the new WebIDL bindings it will be able to participate in cycles, and if the observer service is holding a strong reference to the plugin array any garbage cycle it participates in cannot be freed by the cycle collector.  If an nsPluginArray participates in a cycle with its Navigator it will not be broken until shutdown, because the code that would release the last external reference to the plugin array runs from Navigator's dtor.

Comment 10

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2

Ok - now nsPluginArray registers itself as a weak observer, so there shouldn't be any cycle problems.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 734875 [details] [diff] [review]
patch v2

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

::: dom/base/nsPluginArray.cpp
@@ +39,5 @@
> +{
> +  nsCOMPtr<nsIObserverService> obsService =
> +    mozilla::services::GetObserverService();
> +  if (obsService)
> +    obsService->AddObserver(this, "plugin-info-updated", true);

Braces.

@@ +289,1 @@
>  //

Go ahead and remove this while you're here.

Comment 12

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2.1

Addressed comments, carrying over r+.
This looked pretty good: https://tbpl.mozilla.org/?tree=Try&rev=b79d6e0e2df9
I'll land when the trees open up again.

Comment 13

[Dana Keeler (she/her) [:keeler]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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