gBrowser is not defined in browser-syncui.js

RESOLVED FIXED in mozilla8

Status

()

defect
RESOLVED FIXED
8 years ago
10 months ago

People

(Reporter: zpao, Assigned: gps)

Tracking

unspecified
mozilla8
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

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...
Posted patch Patch v0.1 (obsolete) — Splinter Review
I should actually test this with a profile that has sync setup, but I think this should do the trick.
Assignee: nobody → paul
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.
(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!
Attachment #541125 - Flags: review?(gavin.sharp)
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.
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
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?
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?
(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...
(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)?
(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 :)
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.
Assignee: paul → gps
Attachment #541125 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #549272 - Flags: review?(philipp)
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.
Attachment #549272 - Flags: review?(philipp) → review-
Incorporated feedback from review on last patch.
Attachment #549272 - Attachment is obsolete: true
Attachment #549525 - Flags: review?(philipp)
Comment on attachment 549525 [details] [diff] [review]
Unload observers on window unload v2

rock.
Attachment #549525 - Flags: review?(philipp) → review+
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.
Whiteboard: [fixed in services][qa-]
This is identical to the previous patch but with a review in summary line.
Attachment #549525 - Attachment is obsolete: true
Oh, philikon already committed. Ignore comment #16.
http://hg.mozilla.org/mozilla-central/rev/5b15978e85f7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla8
Duplicate of this bug: 635125
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.