833292 - TypeError: this.chatbar.removeAll is not a function

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

When running the sessionstore test suite I see a lot of these:

> [Exception... "'TypeError: this.chatbar.removeAll is not a function' when calling method: [nsIRunnable::run]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

Comment 1

[:Gavin Sharp [email: [e-mail]]]

That probably occurs when windows are being opened/close rapidly. One of the SocialChatBar.update callers may be running when not expected.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: don't run SocialUI initialization code when called after the window has been closed

There were a couple of new errors in combination with PBU.isWindowPrivate() calls to a closed window which are caused by this issue as well. No more errors when running sessionstore tests for me.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Alternative which also cleans up more social code

I don't want to tread on any toes, but I'm attaching an alternative patch that, while bigger, actually cleans up some of the social code.

Currently social uses a callback to Social.init(), and as a result of this, has some complexity to avoid sending observer notifications as part of the init process.  This is the root cause of the problem here - that the callback still fires even after window unloads.

This patch cleans that up.  It removes the Social.init() callback and relies purely on the observer notifications.  This means the hacks to not notify during Social.init() are removed, and seeing all observers are removed during unload, it's impossible for the failing code to run after unload (ie, it also solves the underlying problem here).

It might make sense to split this out into a new bug, but this change has been in the back of my mind for a while and this bug/attachment prompted me to do it as it solves this problem without an explicit "_initialized" member.

Over-requesting feedback as usual :)  Tim:  The originally reported message doesn't appear when running all tests in browser/base/content/test, which looks like where the sessionstore tests are - please correct me if I'm wrong.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond (:markh) from comment #3)
> I don't want to tread on any toes, but I'm attaching an alternative patch
> that, while bigger, actually cleans up some of the social code.

Awesome, thanks for stepping in. I don't mind if we throw away my patch, I just want to see this fixed :)

> Over-requesting feedback as usual :)  Tim:  The originally reported message
> doesn't appear when running all tests in browser/base/content/test, which
> looks like where the sessionstore tests are - please correct me if I'm wrong.

The tests are located in browser/components/sessionstore/test/. I'll apply your patch and report back.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looks good, no errors here.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 715269 [details] [diff] [review]
Alternative which also cleans up more social code

I like this, especially finally cleaning up the init.  Is this all necessary to fix this bug, or should it be done separately?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)

> I like this, especially finally cleaning up the init.  Is this all necessary
> to fix this bug, or should it be done separately?

I'm not sure what you are asking here, but the original patch fixes the bug, so it could be argued this is not strictly necessary to fix it.  However, this is a cleaner way of fixing the bug and the only practical way of doing it without introducing a new "_initialized" state variable.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=ca60a27fa710

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3892ce628367

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3892ce628367

Comment 10

[u279076]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/3892ce628367

This has caused a regression, see bug 847825.