Last Comment Bug 639970 - Error: "document is null" / "Weave is not defined" in syncUI.js
: Error: "document is null" / "Weave is not defined" in syncUI.js
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Sync UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.5
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 666330
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-08 12:55 PST by Igor Velkov
Modified: 2011-08-04 11:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
patch (3.58 KB, patch)
2011-08-02 12:41 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
patch v2 (3.97 KB, patch)
2011-08-02 14:55 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Review
patch v2a (3.76 KB, patch)
2011-08-03 11:04 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
patch v2b [Checkin: comments 13 and 18] (3.71 KB, patch)
2011-08-03 13:11 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Review

Description Igor Velkov 2011-03-08 12:55:56 PST
Time-to time, but not always, when sync occures by timer (not by "sync now" button), I see in error console:

Error: Weave is not defined
Source file: chrome://communicator/content/sync/syncUI.js
Line: 385

it's
_onSyncEnd: function SUI__onSyncEnd(success) 
...
       let notification =
        new Weave.Notification(title, description, null, priority, buttons);
      Weave.Notifications.replaceTitle(notification);
    }
    else {
      // Clear out sync failures on a successful sync
385:      Weave.Notifications.removeAll(title);
    }
Comment 1 Igor Velkov 2011-03-08 13:19:21 PST
Don't know is in realted or not:
also time-to-time I see a chains of
Error: document is null
Source file: chrome://communicator/content/sync/syncUI.js
Line: 170
 ----------
Error: Weave is not defined
Source file: chrome://communicator/content/sync/syncUI.js
Line: 385

just now - 3 times in one sequence.
Comment 2 Igor Velkov 2011-03-08 13:19:40 PST
It's all about 
Mozilla/5.0 (Windows NT 5.1; rv:2.0b13pre) Gecko/20110308 Firefox/4.0b13pre SeaMonkey/2.1b3pre - Build ID: 20110308003005
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-03-08 14:50:05 PST
Looks like this happens when a window that initiated a Sync run is closed before Sync finishes. I could reproduce it like this (assuming Sync is set up):
1. Start SM with only MailNews open
2. Open a browser window
3. Wait for the MailNews window to initiate a Sync run (happens automatically)
4. Close the MailNews window before the Sync run finishes.

Line 170 is in method onActivityStart which is called from the observe method.
Line 385 is in method _onSyncEnd which is called from onSyncFinish (and onSyncError), which is again called from the observe method.

The corresponding observers are added in method initUI, but AFAICS they are never removed (initUI itself is called from the observe method for the weave:service:ready observer which is removed after having been called once).

I'm not too familiar with observers and their removal (i.e. whether removing an observer from "somewhere" would actually fix these problems here), but maybe we can add a check for "Weave" and/or "document" in the observe method (and just return if it's null). I'm just a concerned that this might make things worse. :-/
Comment 4 Karsten Düsterloh 2011-06-25 12:52:43 PDT
Not sure if this is the same bug:
- Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 SeaMonkey/2.4a1
- profile without Sync set up
- Preferences → Sync → Set up Sync, then Connect
- click Cancel
Result:
Error: Weave is not defined
Source File: chrome://communicator/content/sync/syncSetup.js
Line: 574
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-06-25 13:02:11 PDT
(In reply to comment #4)
> Not sure if this is the same bug:

It's not really the same bug (different file, different circumstances) but the conditions under which they happen are similar: In your case, the Weave object seems to be bound to the wizard, which is probably gone by the time the onAbort function (in which the error happens) is actually executed. In the case of this bug, a Sync run is initiated from a window that is gone by the time the asynchronous call of the _onSyncEnd method is executed.
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-08-02 12:41:29 PDT
Created attachment 550169 [details] [diff] [review]
patch

So the Firefox guys noticed this, too. This ports their bug 666330.
Comment 7 neil@parkwaycc.co.uk 2011-08-02 13:26:04 PDT
Comment on attachment 550169 [details] [diff] [review]
patch

Hmm... if the window is unloaded, then Components won't exist either... although at least the error will be more indicative of the problem ;-)
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-08-02 14:55:43 PDT
Created attachment 550224 [details] [diff] [review]
patch v2

(Not obsoleting the other patch until I have a feedback on this one.)

When I ran with my previous patch, I got this in the Error Console after starting with MailNews, opening the browser, closing MailNews, opening MailNews and closing MailNews:

Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
chrome://communicator/content/sync/syncUI.js line 77

which pointed at this line:
  Services.obs.removeObserver(gSyncUI, topic);

That got me thinking. It turned out that the observers had not been set at all when MailNews was reopened. The observers are added in initUI(), which is called from observe() with topic "weave:service:ready". So that notification didn't fire, or better: Not again.

I've added a code branch that calls initUI() directly from init() if Weave/Sync is already in the ready state. With that, I also fixed a long-standing issue I saw: Until now, the Sync throbber (of the customizable toolbar item) only worked in either MailNews *or* the browser, depending which opened first. Also the tooltip (which tells when Sync last ran) sometimes got lost. Possibly the same applied to the error notification bar (haven't checked).

Neil, if you r+ this I'd like to get this into Aurora and Beta, too. Otherwise I'd have to move it to a separate bug.
Comment 9 neil@parkwaycc.co.uk 2011-08-02 16:05:53 PDT
Comment on attachment 550224 [details] [diff] [review]
patch v2

Is it me or is Firefox buggy in that their fix only applies to their first window (i.e. the one that starts Sync)?

>-    Services.obs.addObserver(this, "weave:service:ready", true);
>+    if (Weave.Status.ready) {
>+      // The notification in the else branch is only sent once per session,
>+      // i.e. it only works for the window that triggered the init.
>+      this.initUI();
>+    } else {
>+      // This will be the first notification fired during init.
>+      // We can set up everything else later.
>+      Services.obs.addObserver(this, "weave:service:ready", true);
>+    }
> 
>     // Remove the observer if the window is closed before the observer
>     // was triggered.
>     window.addEventListener("unload", function SUI_unload() {
>+      gSyncUI._unloaded = true;
>       window.removeEventListener("unload", SUI_unload, false);
>       Services.obs.removeObserver(gSyncUI, "weave:service:ready");
You need to be careful not to remove this observer if you didn't add it. One approach is to always add the observer. Another approach is to tweak the _obs array if you add the observer, thus ensuring that you remove it conditionally.
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-08-03 11:04:31 PDT
Created attachment 550432 [details] [diff] [review]
patch v2a

(In reply to comment #9)
> Is it me or is Firefox buggy in that their fix only applies to their first
> window (i.e. the one that starts Sync)?

If I'm in a good mood I may let them know. ;-)

> You need to be careful not to remove this observer if you didn't add it.

Good point.

> One approach is to always add the observer.

That looks cleanest and spares me adapting yet another comment. ;-)
Comment 11 neil@parkwaycc.co.uk 2011-08-03 12:02:07 PDT
Comment on attachment 550432 [details] [diff] [review]
patch v2a

>   observe: function SUI_observe(subject, topic, data) {
>+    if (this._unloaded) {
>+      Components.utils.reportError("SyncUI observer called after unload: " + topic);
>+      return;
[Thinking about it, I would possibly be slightly happier with
   throw "SyncUI observer called after unload: " + topic;
 as this doesn't depend on having Components in scope.]
Comment 12 Jens Hatlak (:InvisibleSmiley) 2011-08-03 13:11:46 PDT
Created attachment 550473 [details] [diff] [review]
patch v2b [Checkin: comments 13 and 18]

(In reply to comment #11)
> [Thinking about it, I would possibly be slightly happier with
>    throw "SyncUI observer called after unload: " + topic;
>  as this doesn't depend on having Components in scope.]

Sure, why not, also saves us two lines. ;-)

Requesting approval for a safe patch that fixes Sync in any but the first opened window (in addition to addressing the original topic of this bug, getting rid of some "triggering window already gone" errors).
Comment 13 Jens Hatlak (:InvisibleSmiley) 2011-08-03 13:45:17 PDT
Comment on attachment 550473 [details] [diff] [review]
patch v2b [Checkin: comments 13 and 18]

http://hg.mozilla.org/comm-central/rev/7325a5a4b19b
Comment 14 Justin Wood (:Callek) 2011-08-03 14:09:46 PDT
Comment on attachment 550473 [details] [diff] [review]
patch v2b [Checkin: comments 13 and 18]

Before we approve this, can you verify that it does, indeed, affect beta/aurora. And that this patch does indeed work, and fixes it on beta/aurora. (The dependant mozilla bug is not landed on their end for Gecko 6/7)
Comment 15 Jens Hatlak (:InvisibleSmiley) 2011-08-03 23:45:43 PDT
(In reply to comment #14)
> Before we approve this, can you verify that it does, indeed, affect
> beta/aurora. And that this patch does indeed work, and fixes it on
> beta/aurora. (The dependant mozilla bug is not landed on their end for Gecko
> 6/7)

All issues that are fixed here have been in place since I introduced Sync to SeaMonkey (in terms of stable releases: 2.1). I just didn't know exactly where they were coming from and how to fix them--until now.

Basically there are 10 kinds of Sync bugs: Those that go with Sync/Weave back-end changes, and those that don't. This bug is of the latter kind (i.e. it's all in UI/front-end land), so it doesn't matter in which Mozilla/Gecko/platform version the Firefox bug is fixed. We can make our choice independently (and in this case, we should!).
Comment 16 Justin Wood (:Callek) 2011-08-03 23:53:04 PDT
Comment on attachment 550473 [details] [diff] [review]
patch v2b [Checkin: comments 13 and 18]

we'll be spinning last beta within the next 24 hours, probably earlier, so land the comm-beta asap, please.
Comment 17 Jens Hatlak (:InvisibleSmiley) 2011-08-04 01:43:00 PDT
(In reply to comment #16)
> we'll be spinning last beta within the next 24 hours, probably earlier, so
> land the comm-beta asap, please.

~8h from now earliest for me. Anyone feel free to land sooner.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2011-08-04 11:14:07 PDT
Comment on attachment 550473 [details] [diff] [review]
patch v2b [Checkin: comments 13 and 18]

http://hg.mozilla.org/releases/comm-aurora/rev/c22150b0c74a
http://hg.mozilla.org/releases/comm-beta/rev/fc6c35d548ea

Note You need to log in before you can comment on or make changes to this bug.