Last Comment Bug 711173 - Fix for UI freeze on feed biff/get new messages; renovate newsblog.js for fun.
: Fix for UI freeze on feed biff/get new messages; renovate newsblog.js for fun.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: alta88
:
Mentors:
: 521535 733908 (view as bug list)
Depends on: 709247
Blocks: 267082 293289 308435 337363 365377 388919 551144 571760 692318 716706
  Show dependency treegraph
 
Reported: 2011-12-15 10:51 PST by alta88
Modified: 2012-03-08 11:22 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch. (32.35 KB, patch)
2011-12-15 10:51 PST, alta88
no flags Details | Diff | Splinter Review
address comments. (32.90 KB, patch)
2011-12-17 07:51 PST, alta88
no flags Details | Diff | Splinter Review
updated to address nits to date. (33.08 KB, patch)
2012-02-07 11:18 PST, alta88
mozilla: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta-
Details | Diff | Splinter Review

Description alta88 2011-12-15 10:51:19 PST
Created attachment 582030 [details] [diff] [review]
patch.

this patch attempts the following:
1) fix UI freeze on new messages using an enumerator/yield method for each feed url in each folder.
2) unsubscribe a deleted folder (deleted *and* moved to trash), depends on Bug 709247.
3) resync feeds.rdf db with feedUrl (one time deal), see Bug 709247.

along the way it:
1) now allows getting new messages to work on a folder and all its subfolders.
2) creates a FeedUtils object and moves some vars/functions there.
3) converts newsblog.js to using Services and MailServices helpers.
4) begin to replace debug with the nice log4moz logger.

useful to do?
1) convert newsblog.js to modern xpcom registration methods.
Comment 1 Ludovic Hirlimann [:Usul] 2011-12-16 00:37:11 PST
I don't see any test are these changes not worth having some tests ?
Comment 2 Ludovic Hirlimann [:Usul] 2011-12-16 08:47:42 PST
Alta do you plan to switch to the feed reader of firefox too ?
Comment 3 alta88 2011-12-16 10:18:07 PST
(In reply to Ludovic Hirlimann [:Usul] from comment #2)
> Alta do you plan to switch to the feed reader of firefox too ?

while it would be worthy, that's a Real Project.  imo it should be done with the idea of taking Feeds in Tb to the next level, and would need buyin and interest from moz.
Comment 4 neil@parkwaycc.co.uk 2011-12-16 15:25:19 PST
Comment on attachment 582030 [details] [diff] [review]
patch.

Unfortunately there are too many unrelated changes for me to be able to make sense of the whole patch at once.

>+if (Cc == undefined)
>+  var Cc = Components.classes;
>+if (Ci == undefined)
>+  var Ci = Components.interfaces;
>+if (Cr == undefined)
>+  var Cr = Components.results;
>+if (Cu == undefined)
>+  var Cu = Components.utils;
This is the sort of thing that gives Cx a bad name...

>+Cu.import("resource://gre/modules/mailServices.js");
mailServices doesn't live in the GRE.

>+  get log() {
>+    let logger = Log4Moz.getConfiguredLogger("Feeds");
>+    this.__defineGetter__("log", function() { return logger });
>+    return this.log;
>+  },
XPCOMUtils has some methods to help you do this.

>+    // Return if there are no feedUrls for the base folder in the feeds
>+    // database, the base folder has no subfolders, or the folder is in Trash.
Why if no subfolders?

>+          msgDb = folder.QueryInterface(Ci.nsIMsgLocalMailFolder)
>+                        .getDatabaseWOReparse();
I still think folder.msgDatabase is better.
Comment 5 alta88 2011-12-17 07:51:28 PST
Created attachment 582532 [details] [diff] [review]
address comments.
Comment 6 Ian Neal 2011-12-17 10:24:45 PST
Comment on attachment 582532 [details] [diff] [review]
address comments.

Just a quick drive by:
> function loadScripts()
> {
>-  var scriptLoader =  Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>+  var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                      .getService(Components.interfaces.mozIJSSubScriptLoader);
Any reason why you are not using Services.scriptloader or is it that Services.jsm is not available in this context?
Comment 7 Ian Neal 2011-12-17 10:54:50 PST
Comment on attachment 582532 [details] [diff] [review]
address comments.


>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Doesn't the loading of Services.jsm pull in XPCOMUtils.jsm already?
Comment 8 alta88 2011-12-17 14:30:28 PST
(In reply to Ian Neal from comment #6)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> Just a quick drive by:
> > function loadScripts()
> > {
> >-  var scriptLoader =  Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> >+  var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> >                      .getService(Components.interfaces.mozIJSSubScriptLoader);
> Any reason why you are not using Services.scriptloader or is it that
> Services.jsm is not available in this context?

it wouldn't work using Services.  i tried it a number of way, passing in a context, etc.  no errors just no functions from imported files.  

(In reply to Ian Neal from comment #7)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> 
> >+Cu.import("resource://gre/modules/Services.jsm");
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Doesn't the loading of Services.jsm pull in XPCOMUtils.jsm already?

i believe only in its own scope.  anyway, removing it makes the Subscribe dialog blow up with 'not defined'.
Comment 9 David :Bienvenu 2011-12-20 11:11:02 PST
I will review this over the holidays, but I just wanted to say that I think it's a really useful change.
Comment 10 David :Bienvenu 2011-12-28 09:00:49 PST
moving the aSubscribeMode ? inside the GetStringFromName call would be slightly cleaner:

+          this.mStatusFeedback.showStatusString(aSubscribeMode ?
+            FeedUtils.strings.GetStringFromName('subscribe-validating-feed') :
+            FeedUtils.strings.GetStringFromName('newsblog-getNewMsgsCheck'));

I'll run with the patch for a while - I subscribed to a feed w/o a problem (though I had to use the subscribe UI, not drag drop, but I've had that problem before)
Comment 11 alta88 2011-12-28 11:07:29 PST
(In reply to David :Bienvenu from comment #10)
> moving the aSubscribeMode ? inside the GetStringFromName call would be
> slightly cleaner:
> 
> +          this.mStatusFeedback.showStatusString(aSubscribeMode ?
> +           
> FeedUtils.strings.GetStringFromName('subscribe-validating-feed') :
> +           
> FeedUtils.strings.GetStringFromName('newsblog-getNewMsgsCheck'));
> 
GetStringFromName is a function that comes with a Services.strings bundle instance, so i'm not sure how that would work.  this existed before; do you mean breaking it out, like
string = aSubscribeMode ? GetSFN("name1") : GetSFN("name2");

> I'll run with the patch for a while - I subscribed to a feed w/o a problem

also, the one liner in bug 709247 is needed to make sure folderpane moved/deletes are happy.

a note: this bug and bug 709247 should be in the same branch as bug 705504, so the feed db resync can happen.

> (though I had to use the subscribe UI, not drag drop, but I've had that
> problem before)

i think dnd of a url on a folderpane feed folder malforms the url (adds 'Subscribe' to the end).
Comment 12 David :Bienvenu 2011-12-28 16:20:19 PST
No, I mean, this.mStatusFeedback.showStatusString(FeedUtils.strings.GetStringFromName(aSubscribeMode ? 'subscribe-validating-feed' : 'newsblog-getNewMsgsCheck');
Comment 13 neil@parkwaycc.co.uk 2012-01-10 06:30:00 PST
Comment on attachment 582532 [details] [diff] [review]
address comments.

I had another look at this and I saw a few potential issues:
1. gNumPendingFeedDownloads should be renamed mNumPendingFeedDownloads.
2. gNumPendingFeedDownloads should be a property of the progress notifier.
3. I don't see the point of making the progress notifier a sub-object.
4. I don't see the point of the feedStringProperties property.
Comment 14 alta88 2012-01-11 08:09:27 PST
(In reply to neil@parkwaycc.co.uk from comment #13)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> I had another look at this and I saw a few potential issues:
> 1. gNumPendingFeedDownloads should be renamed mNumPendingFeedDownloads.
ok.
> 2. gNumPendingFeedDownloads should be a property of the progress notifier.
ok.
> 3. I don't see the point of making the progress notifier a sub-object.
there is tremendous namespace pollution in feeds code. this function (duplicated somewhat in subscribe) as well as all the constants and functions in utils.js should go into the FeedUtils object.
> 4. I don't see the point of the feedStringProperties property.
meaning it should be hardcoded in the getter?  that style isn't preferable, imo.  else i don't understand the issue.
Comment 15 alta88 2012-01-11 12:16:51 PST
*** Bug 521535 has been marked as a duplicate of this bug. ***
Comment 16 neil@parkwaycc.co.uk 2012-01-15 14:53:17 PST
(In reply to alta88 from comment #14)
> (In reply to comment #13)
> > 3. I don't see the point of making the progress notifier a sub-object.
> there is tremendous namespace pollution in feeds code. this function
> (duplicated somewhat in subscribe) as well as all the constants and
> functions in utils.js should go into the FeedUtils object.
The progress notifier isn't a function.

> > 4. I don't see the point of the feedStringProperties property.
> meaning it should be hardcoded in the getter?  that style isn't preferable,
> imo.  else i don't understand the issue.
MXR for Services.strings.createBundle within all of comm-central returns 101 hits. Admittedly some of them are split across lines so I can't readily see what their style is, but apart from two that use a function parameter and six that use a global constant, the rest all seem to hardcode it.
Comment 17 alta88 2012-02-06 12:47:36 PST
david, have you had a chance to look at this?
Comment 18 David :Bienvenu 2012-02-06 12:49:46 PST
I'll try to look at it in the next couple days - I was waiting for you and Neil to resolve the issues you were discussing...
Comment 19 alta88 2012-02-06 14:19:46 PST
i was thinking to roll those into an updated patch with whatever comments you also had as they don't affect the underlying core changes..  really the substantive thing is whether there is any tuning to be done to the yield method to stop the UI freeze.
Comment 20 David :Bienvenu 2012-02-06 15:04:16 PST
(In reply to alta88 from comment #19)
> i was thinking to roll those into an updated patch with whatever comments
> you also had as they don't affect the underlying core changes..  really the
> substantive thing is whether there is any tuning to be done to the yield
> method to stop the UI freeze.

ah, ok, then my https://bugzilla.mozilla.org/show_bug.cgi?id=711173#c10 still applies...other than that, instead of a single yield, I was thinking you could chain the requests, i.e., not start one until the previous one had finished. Otherwise, you could get 50 http servers sending you data all at once, and essentially lock up the UI. In other words, I'm not sure that just yielding while kicking off the downloads will really help that much. I can try running with it later.
Comment 21 alta88 2012-02-06 17:10:11 PST
without this patch, i find a 5 second total UI freeze (Not responding in the titlebar in addition) on a biff with 50 feeds urls; with it i notice there is no longer any freeze (test profile with 5 accounts, the largest of which is 50).

the async handling of all the nsIXMLHttpRequest returns doesn't seem to be a bottleneck, but this is the sort of thing to identify here.  it would be a bit more complex to single thread them and wait before launching the next.
Comment 22 Michael Sirolly 2012-02-06 18:48:54 PST
Disclosure: I submitted duplicate bug 521535; and, I'm not a developer.

You probably wouldn't want individual feed requests hinging on previous requests -- when a server for a request is slow to respond, non-responsive or unreachable, then what?

Requests might be dispatched in rolling intervals, though -- every second, every 5 seconds, what have you. Don't know if Thunderbird's code offers a platform-independent check of processor utilization, but that would be a good determinant. Don't you hate waking a computer when you need to do something productive, only to have it immediately brought to its knees by virus update checks, software update checks, email checks, file backups and so on? Apps ought to be good citizens, and avoid piling on and saturating a system that's pinned or nearly pinned.

Perhaps consider what a user wants by specifying "Check for new articles every (2/5/15/30/60...) minutes." Check all 50 feeds at the same instant? Check one feed each "# of minutes divided by # of feeds" interval, setting off the new message indicator/sound each time? Of course, smartest might be to check the most recently active feeds more often (e.g., an active Twitter conversation), and less active feeds less often (e.g. a personal blog), but that's another enhancement request entirely. My two cents, thanks for listening!   :-)
Comment 23 alta88 2012-02-07 11:18:35 PST
Created attachment 595108 [details] [diff] [review]
updated to address nits to date.
Comment 24 alta88 2012-02-07 11:51:51 PST
(In reply to Michael Sirolly from comment #22)
> My two cents, thanks for listening!   :-)

yes, the immediate freeze upon unlocking the screen was the last straw..

an intelligent algo for dispatching would undoubtedly bring an incremental improvement, but it might be small.  this bug is meant to really triage the lock condition, which imo is 90% (fuzzy) of the bad UX.  simply having a greater amount of time (20ms eg) returned to the main thread between yields could reduce the rest.

it would be a great enhancement to add a per feed refresh interval rather than per account, but this means adding a per feed field and some rearchitecture, and that leads to a bigger project, imo.  with questions like, shouldn't the feeds dbs really be in sqlite, etc etc.
Comment 25 David :Bienvenu 2012-02-10 07:18:17 PST
Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

OK, thx, yeah, I'm sure that opening the db's for each feed in a tight loop is mostly responsible for the hang, and that yielding between each will give the UI a little bit of a chance to breath, and it should be a worthwhile improvement.
Comment 26 alta88 2012-02-10 12:06:19 PST
checkin-needed.

this bug should be in the same branch as bug 709247, which currently is Tb11 (due to the feed db resync being here).  so either this should go to beta, or bug 709247 should be backed out of beta for aurora (Tb12) and this bug advanced to aurora from trunk..?
Comment 27 David :Bienvenu 2012-02-10 12:08:16 PST
Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

alta88 - that means we should request approval comm-aurora and comm-beta, right? And if denied, then we have to back out the other back from those branches?
Comment 28 alta88 2012-02-10 12:44:28 PST
checkin-needed.

this bug should be in the same branch as bug 709247, which currently is Tb11 (due to the feed db resync being here).  so either this should go to beta, or bug 709247 should be backed out of beta for aurora (Tb12) and this bug advanced to aurora from trunk..?(In reply to David :Bienvenu from comment #27)
> Comment on attachment 595108 [details] [diff] [review]
> updated to address nits to date.
> 
> alta88 - that means we should request approval comm-aurora and comm-beta,
> right? And if denied, then we have to back out the other back from those
> branches?

yes.  the goal would be to have both bugs hit release at the same time.
Comment 29 Mark Banner (:standard8) 2012-02-13 13:01:15 PST
Checked in: http://hg.mozilla.org/comm-central/rev/42b7c8b861b4
Comment 30 Mark Banner (:standard8) 2012-02-20 02:12:06 PST
(In reply to alta88 from comment #26)
> checkin-needed.
> 
> this bug should be in the same branch as bug 709247, which currently is Tb11
> (due to the feed db resync being here).  so either this should go to beta,
> or bug 709247 should be backed out of beta for aurora (Tb12) and this bug
> advanced to aurora from trunk..?

If we backed out bug 709247 from beta, would we also have to back out bug 705504?
Comment 31 alta88 2012-02-20 07:47:59 PST
argh, a copy/paste slip, i meant bug 705504 should be in the same branch/backed out, yes.  bug 709247 should remain as it fixes an error going forward.

thanks for the catch.
Comment 32 Mark Banner (:standard8) 2012-02-21 14:12:49 PST
Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

Ok, so I think due to the level of change here, then I'd rather this have a slightly longer period for testing. Hence not taking for aurora, but will for beta. We'll back out bug 705504 from beta.
Comment 33 Mark Banner (:standard8) 2012-02-21 14:14:26 PST
Checked in to branch for TB 12: http://hg.mozilla.org/releases/comm-aurora/rev/958575ce81d0
Comment 34 alta88 2012-03-08 11:22:42 PST
*** Bug 733908 has been marked as a duplicate of this bug. ***

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