Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix for UI freeze on feed biff/get new messages; renovate newsblog.js for fun.

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

Trunk
Thunderbird 13.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird12 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #582030 - Flags: review?(neil)
Attachment #582030 - Flags: review?(dbienvenu)
I don't see any test are these changes not worth having some tests ?
(Assignee)

Updated

6 years ago
Blocks: 692318
(Assignee)

Updated

6 years ago
Blocks: 267082
(Assignee)

Updated

6 years ago
Blocks: 293289
(Assignee)

Updated

6 years ago
Blocks: 308435
(Assignee)

Updated

6 years ago
Blocks: 337363
(Assignee)

Updated

6 years ago
Blocks: 365377
(Assignee)

Updated

6 years ago
Blocks: 551144
(Assignee)

Updated

6 years ago
Blocks: 571760
Alta do you plan to switch to the feed reader of firefox too ?
(Assignee)

Comment 3

6 years ago
(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

6 years ago
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.
Attachment #582030 - Flags: review?(neil)
(Assignee)

Comment 5

6 years ago
Created attachment 582532 [details] [diff] [review]
address comments.
Attachment #582030 - Attachment is obsolete: true
Attachment #582030 - Flags: review?(dbienvenu)
Attachment #582532 - Flags: review?(neil)
Attachment #582532 - Flags: review?(dbienvenu)

Comment 6

6 years ago
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

6 years ago
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?
(Assignee)

Comment 8

6 years ago
(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

6 years ago
I will review this over the holidays, but I just wanted to say that I think it's a really useful change.

Comment 10

6 years ago
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)
(Assignee)

Comment 11

6 years ago
(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

6 years ago
No, I mean, this.mStatusFeedback.showStatusString(FeedUtils.strings.GetStringFromName(aSubscribeMode ? 'subscribe-validating-feed' : 'newsblog-getNewMsgsCheck');
(Assignee)

Updated

6 years ago
Blocks: 716706
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.
(Assignee)

Comment 14

6 years ago
(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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 521535
(Assignee)

Updated

6 years ago
Blocks: 388919
(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.
(Assignee)

Comment 17

6 years ago
david, have you had a chance to look at this?

Comment 18

6 years ago
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...
(Assignee)

Comment 19

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
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

6 years ago
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!   :-)
(Assignee)

Comment 23

6 years ago
Created attachment 595108 [details] [diff] [review]
updated to address nits to date.
Attachment #582532 - Attachment is obsolete: true
Attachment #582532 - Flags: review?(neil)
Attachment #582532 - Flags: review?(dbienvenu)
Attachment #595108 - Flags: review?(dbienvenu)
(Assignee)

Comment 24

6 years ago
(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

6 years ago
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.
Attachment #595108 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 26

6 years ago
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..?
Keywords: checkin-needed

Comment 27

6 years ago
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?
Attachment #595108 - Flags: approval-comm-beta?
Attachment #595108 - Flags: approval-comm-aurora?
(Assignee)

Comment 28

6 years ago
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.
Checked in: http://hg.mozilla.org/comm-central/rev/42b7c8b861b4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(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?
(Assignee)

Comment 31

6 years ago
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 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.
Attachment #595108 - Flags: approval-comm-beta?
Attachment #595108 - Flags: approval-comm-beta-
Attachment #595108 - Flags: approval-comm-aurora?
Attachment #595108 - Flags: approval-comm-aurora+
Checked in to branch for TB 12: http://hg.mozilla.org/releases/comm-aurora/rev/958575ce81d0
status-thunderbird12: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 733908
You need to log in before you can comment on or make changes to this bug.