666330 - gBrowser is not defined in browser-syncui.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Silly me, trying to access gBrowser off the assumed global when it isn't defined doesn't work.

This might be the same as bug 636947...

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

I should actually test this with a profile that has sync setup, but I think this should do the trick.

Comment 2

[Philipp von Weitershausen [:philikon]]

Thanks for the patch, Paul. I'm curious when you are seeing this and how you've verified that your patch actually fixes the problem, particularly since there are no tests as you mention. Do you have STRs that would allow us to reproduce?

In most cases (bug 636947 seems to be no exception), the bug is actually due to an observer or event handler that wasn't unregistered properly. So I'd rather try to investigate the root cause rather than applying a wall paper fix that will potentially even make the root cause harder to detect.

Comment 3

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

(In reply to comment #2)
> In most cases (bug 636947 seems to be no exception), the bug is actually due
> to an observer or event handler that wasn't unregistered properly. So I'd
> rather try to investigate the root cause rather than applying a wall paper
> fix that will potentially even make the root cause harder to detect.

I concur!

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I also concur! I haven't figured out what's causing these error messages. I'm seeing other ones too (Weave is not defined, this._stringBundle is not defined) but didn't look big picture. I'll see if I can narrow this down to something we're doing.

Comment 5

[Martijn Wargers (dead)]

I'm seeing these errors too in Firefox 5, typically after a while (after GC?), having multiple browser windows open.

Fout: gBrowser is not defined
Bronbestand: chrome://browser/content/browser.js
Regel: 5247

Fout: Cc is not defined
Bronbestand: chrome://browser/content/browser.js
Regel: 5545

Fout: this._stringBundle is undefined
Bronbestand: chrome://browser/content/browser.js
Regel: 5411

Fout: Weave is not defined
Bronbestand: chrome://browser/content/browser.js
Regel: 5480

Comment 6

[Philipp von Weitershausen [:philikon]]

I have a suspicion what's causing these. gSyncUI registers its observers as weak observers, but it never actually removes them when the window gets unloaded. So possibly they stick around somehow?

Comment 7

[Gregory Szorc [:gps]]

I'm trying to wrap my head around how all this stuff works. Would it be safe to modify the "unload" listener in gSyncUI to set a flag in gSyncUI then have every handler noop if this flag is set?

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> I'm trying to wrap my head around how all this stuff works. Would it be safe
> to modify the "unload" listener in gSyncUI to set a flag in gSyncUI then
> have every handler noop if this flag is set?

Or why not simply remove the observers in the unload listener...

Comment 9

[Gregory Szorc [:gps]]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm trying to wrap my head around how all this stuff works. Would it be safe
> > to modify the "unload" listener in gSyncUI to set a flag in gSyncUI then
> > have every handler noop if this flag is set?
> 
> Or why not simply remove the observers in the unload listener...

Even better.

I'd love to submit a test that reproduces this. But, that might be beyond my current ninja skills. I'm thinking an appropriate test would have to instantiate and init gSyncUI, remove references to it, then force a GC. The forcing a GC bit I'm not sure about. Can we manually trigger a GC from JS?

Would you r+ a patch without a unit test that reproduces this bug (but with test(s) that verify listeners are gone)?

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> I'd love to submit a test that reproduces this. But, that might be beyond my
> current ninja skills. I'm thinking an appropriate test would have to
> instantiate and init gSyncUI, remove references to it, then force a GC. The
> forcing a GC bit I'm not sure about. Can we manually trigger a GC from JS?

Yes. But testing that there's no error executing something we no longer keep a reference to is kind of tricky.

> Would you r+ a patch without a unit test that reproduces this bug (but with
> test(s) that verify listeners are gone)?

I have no idea how to test that observers are really gone, when you really want to them to be gone... I would even r+ a patch without a test :)

Comment 11

[Gregory Szorc [:gps]]

Attachment: Unload observers on window unload

This patch removes registered observers when the window unload event is fired. It should hopefully fix the root cause.

I've tested that Nightly runs properly with this patch and that setting up Sync works. xpcshell-tests for services/sync also all pass.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 549272 [details] [diff] [review]
Unload observers on window unload

>-    window.addEventListener("unload", function() {
>-      window.removeEventListener("unload", arguments.callee, false);
>+    window.addEventListener("unload", function onUnload() {
>+      gSyncUI._unloaded = true;
>+      window.removeEventListener("unload", onUnload, false);
>       Services.obs.removeObserver(gSyncUI, "weave:service:ready");
>+
>+      gSyncUI._obs.forEach(function(topic) {
>+        Services.obs.removeObserver(gSyncUI, topic);
>+      });

It's not quite as simple as this. These observers get added in initUI which will only be called if Sync is set up. If a user hasn't set set up sync, these observers won't exist and removeObserver will throw here. To fix I think you can simply wrap this in a check for Weave.Status.ready.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Unload observers on window unload v2

Incorporated feedback from review on last patch.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 549525 [details] [diff] [review]
Unload observers on window unload v2

rock.

Comment 15

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/services-central/rev/5b15978e85f7

I'm going to set this to [qa-] since there isn't really a good way to reliably reproduce these errors. (At least I haven't found one.) If people end up seeing

  SyncUI observer called after unload: ...

in the JS Error Console in the future, we have failed.

Comment 16

[Gregory Szorc [:gps]]

Attachment: Unload observers on window unload v2

This is identical to the previous patch but with a review in summary line.

Comment 17

[Gregory Szorc [:gps]]

Oh, philikon already committed. Ignore comment #16.

Comment 18

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/5b15978e85f7