495113 - Clicking clear now in activity manager hangs thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Dave Townsend [:mossop]]

Attachment: hang stack

I opened the activity manager and clicked "Clear Now". After 10 minutes with Thunderbird showing the beachball and slowing down my machine I killed it and got the attached report from Apple's crash reporter. After relaunching Thunderbird it appears that the activity manager was cleared anyway.

This was with the nightly build from 25th May 2009.

Comment 1

[Mark Banner (:standard8)]

Yep, that's not good.

Comment 2

[Mark Banner (:standard8)]

Hmm so my nightly build that had been running all day showed the hang. A debug build with about 10 or so items didn't.

So either this is due to having a lot of items or there's something else going on here.

Comment 3

[David :Bienvenu]

I tried this with my windows debug build that had been running for an hour or so - clear now did lock up the app for a while, and breaking into the debugger showed that we were executing a lot of js code. My guess is some sort of n-squared algorithm in the js code when clear now is run. I can poke around the code a bit, I guess...

Comment 4

[David :Bienvenu]

clear now is definitely at least n-squared. For each activity that gets removed, we iterate over the list of existing ui elements looking for the activity that got removed. If we cleared the ui elements first, before telling the activity manager backend to clear everything, that would speed things up, I think. But I've only looked at the activity manager briefly so there may be some reason we didn't already do this.

Comment 5

[Mark Banner (:standard8)]

We still need the activity manager to give notifications of items removed - other code may depend on it.

There's a couple of possibilities, firstly how does download manager do it - activity manager mainly came from that, and I'd be surprised if there isn't code for it.

Secondly, we could store a hash or something by activity id in activity.js and therefore have a fast-lookup.

Comment 6

[David :Bienvenu]

I'm not proposing getting rid of the notifications - but if the activity list has already been cleared out, then it takes no time to process the notifications, because the list is empty.

Comment 7

[David :Bienvenu]

the download manager seems to have a way of knowing if multiple items are being removed - if so, it just rebuilds the UI, instead of deleting things one by one. Unfortunately, we did not copy that part of the download manager, so we can't use the same trick. We could, however, probably just set some state that says ignore notifications and then rebuild afterwards.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I see this hang too - especially with a lot of items in Activity Manager.

Comment 9

[David :Bienvenu]

Attachment: snapshot of approach I was taking

davida suggested bwinton take this over, for which I'm grateful.

For debugging, I tweaked the move copy notifications so that instead of 1 notification, we get 100, which fills up the activity manager nicely when you delete messages. (That's *not* intended to be checked in :-) )

Then I tried to copy what the download manager does, which is rebuild when cleared, instead of listening for all the notifications. I kept getting exceptions thrown from clear when I did this, which I couldn't track down because venkman was broken at the time.

Comment 10

[David :Bienvenu]

bwinton, do you think you could look into this? I've attach a wip for one possible approach. Thx!

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Certainly!

Comment 12

[David :Bienvenu]

Here's the not very useful js error I see with my patch:

JavaScript error: , line 0: uncaught exception: [Exception... "'Failure' when ca
lling method: [nsIActivityManager::cleanUp]"  nsresult: "0x80004005 (NS_ERROR_FA
ILURE)"  location: "JS frame :: chrome://messenger/content/activity.js :: clearA
ctivityList :: line 315"  data: no]

But I think it only happens when the indexer is running - if there's not an indexer status thing going on, then clear works fine. Maybe that will help you recreate the issue. Have you turned gloda on?

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Attachment: A fixed-up version of bienvenu's patch.

So I'ld just like to say a little bit about the changes I've made to get the patch working, so that you can see what I did.

The first bug was in mail/components/activity/nsActivityManager.js, where the check for which activities to remove was being done with || instead of &&, thus causing us to try to remove in-progress activities, which was throwing the error you saw.  (There was also a bug there where the call to .state would return "undefined" unless it was in an if-instanceof block.  That was fun to track down.  :)

After that was fixed, I ran into a second bug, where we wouldn't remove the listeners, and then they would get called, but they wouldn't be around anymore.  That's the code that starts with this comment:
+  // since XBL dtors are not working properly when we remove the element,
+  // we have to explicitly remove the binding from activities' listeners
+  // list. See bug 230086 for details.

I also remembered to remove the 100 move copy notifications.  :)

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 383061 [details] [diff] [review]
A fixed-up version of bienvenu's patch.

>diff --git a/mail/components/activity/content/activity.js b/mail/components/activity/content/activity.js
>--- a/mail/components/activity/content/activity.js
>+++ b/mail/components/activity/content/activity.js
>@@ -54,6 +54,7 @@
> let gActivityMgrListener = null;
> let gActivitiesView = null;
> let gActivityLogger = Log4Moz.getConfiguredLogger("activitymgr");
>+let gIgnoreNotifications = false;

This makes me think we should really turn this file into its own object so that we don't polute the global scope with lots of info.

>diff --git a/mail/components/activity/nsActivityManager.js b/mail/components/activity/nsActivityManager.js
>--- a/mail/components/activity/nsActivityManager.js
>+++ b/mail/components/activity/nsActivityManager.js
>@@ -156,11 +156,18 @@
>     // Get the list of aIDs.
>     this.log.info("cleanUp\n");
>     for (var id in this._activities) {
>-      let state = this._activities[id].state;
>-      if (state != Ci.nsIActivityProcess.STATE_INPROGRESS ||
>-          state != Ci.nsIActivityProcess.STATE_PAUSED ||
>-          state != Ci.nsIActivityProcess.STATE_WAITINGFORINPUT ||
>-          state != Ci.nsIActivityProcess.STATE_WAITINGFORRETRY)
>+      let activity = this._activities[id];
>+      if (activity instanceof Ci.nsIActivityProcess) {
>+        // Note: The .state property will return undefined if you aren't in
>+        //       this if-instanceof block.
>+        let state = activity.state;
>+        if (state != Ci.nsIActivityProcess.STATE_INPROGRESS &&
>+            state != Ci.nsIActivityProcess.STATE_PAUSED &&
>+            state != Ci.nsIActivityProcess.STATE_WAITINGFORINPUT &&
>+            state != Ci.nsIActivityProcess.STATE_WAITINGFORRETRY)
>+          this.removeActivity(id);
>+      }
>+      else
>         this.removeActivity(id);

This confuses sendLater.js in the case where we have are sending a message, and we have sent it, but the copy process is still ongoing - because it is expecting the process still to be there. Maybe we can do the process -> event conversion once the send has finished (because the sent is complete, it won't be undone at that stage), and then do the copy process -> event conversion after the copy completes.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: A similar patch, with the code from activity.js in an object, and the sendLater.js fixed.

(In reply to comment #14)
> >--- a/mail/components/activity/content/activity.js
> > let gActivityMgrListener = null;
> > let gActivitiesView = null;
> > let gActivityLogger = Log4Moz.getConfiguredLogger("activitymgr");
> >+let gIgnoreNotifications = false;
> This makes me think we should really turn this file into its own object so
> that we don't polute the global scope with lots of info.

Done, and let me apologize now for the length/complicatedness of the resulting diff.

> >--- a/mail/components/activity/nsActivityManager.js
> This confuses sendLater.js in the case where we have are sending a message,
> and we have sent it, but the copy process is still ongoing - because it is
> expecting the process still to be there. Maybe we can do the process -> event
> conversion once the send has finished (because the sent is complete, it won't
> be undone at that stage), and then do the copy process -> event conversion
> after the copy completes.

Fixed.

Well, kinda.  We don't actually convert the copy process to an event, because if we did, you would get two events for every message.  So we just drop the copy process.  Anyways, it's no longer confused.  (If you'ld rather drop the sending process, and convert the copy process into the event, let me know, and I can switch that around fairly easily.)

Thanks,
Blake.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 384551 [details] [diff] [review]
A similar patch, with the code from activity.js in an object, and the sendLater.js fixed.

>+  gActivityMgrListener: null,
>+  gActivitiesView: null,
>+  gActivityLogger: Log4Moz.getConfiguredLogger("activitymgr"),
>+  gIgnoreNotifications: false,

Can you make these _activit... or _ignore... please - that is how we normally reference internal variables.

> 
>-ActivityMgrListener.prototype = {
>+  selectAll: function()
>+  {
>+    this.gActivitiesView.selectAll();
>+  },

Brackets for functions & within functions should be of the

function () {
  ...
}

style.

>+  Startup: function()
>+  Rebuild: function()
>+  Shutdown: function()

As we're reorganising this file, could you also start these function names (and any others I've not listed) with lower case please - it is what we normally do within js objects.

Apart from those, it is definitely looking a lot better and seems to work fine.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, updated with Standard8's suggestions.

(In reply to comment #16)
> >+  gActivityMgrListener: null,
> Can you make these _activit... or _ignore... please - that is how we normally
> reference internal variables.

Fixed.

> >-ActivityMgrListener.prototype = {
> >+  selectAll: function()
> >+  {
> >+    this.gActivitiesView.selectAll();
> >+  },
> 
> Brackets for functions & within functions should be of the
> function () {
> }
> style.

Fixed.

> >+  Startup: function()
> >+  Rebuild: function()
> >+  Shutdown: function()
> 
> As we're reorganising this file, could you also start these function names (and
> any others I've not listed) with lower case please - it is what we normally do
> within js objects.

Fixed.  Well, mostly.  I left "ActivityMgrListener" capitalized, because we seem to be using it as a class.  (We set its prototype down at the bottom of the file, and call new ActivityMgrListener in the file.)

> Apart from those, it is definitely looking a lot better and seems to work fine.

Great, thanks!  Here's the new patch.
Let me know if there's anything else you'ld like me to change while I'm here.

Later,
Blake.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 384622 [details] [diff] [review]
The previous patch, updated with Standard8's suggestions.

Yep, looks good thanks for the update. r=Standard8.

I've checked this in as well: http://hg.mozilla.org/comm-central/rev/bf741e9ea652