Closed Bug 599119 Opened 13 years ago Closed 13 years ago

Investigate unnecessary plugin directory scans while loading messages, causes pauses/slow display during mail navigation or click

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(blocking-thunderbird3.1 .5+, thunderbird3.1 .5-fixed)

RESOLVED FIXED
Thunderbird 3.3a1
Tracking Status
blocking-thunderbird3.1 --- .5+
thunderbird3.1 --- .5-fixed

People

(Reporter: rain1, Assigned: rain1)

References

()

Details

(Keywords: perf, regression, Whiteboard: [gssolved][gs])

Attachments

(2 files, 1 obsolete file)

Currently on Windows, each time we load a message we scan all directories where there could be plugins twice -- this delays message loads by a very significant amount. To see how bad the cost is, set plugin.scan.plid.all to false.

The call in <http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/base/nsURILoader.cpp#455> (step 1) eventually lands in <http://mxr.mozilla.org/comm-central/source/mozilla/docshell/base/nsWebNavigationInfo.cpp#64>. IsTypeSupportedInternal returns UNSUPPORTED for content type message/rfc822, which causes a plugin dir scan immediately afterwards. Another scan happens at step 2, when the nsMsgWindow (as a registered content listener) passes the request to the docshell content listener <http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgWindow.cpp#436>.

I had a look at nsDocumentViewer (the content viewer for documents) <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp> and it seemed far too complex for us to implement for message/rfc822. Our best bet might be to run the stream converter ourselves and provide text/html to the docshell -- David, is it possible to do so?
blocking-thunderbird3.2: --- → ?
... or, we might implement our own doc viewer factory: <http://mxr.mozilla.org/comm-central/source/mozilla/webshell/public/nsIDocumentLoaderFactory.idl> and do the stream conversion there, if possible.
regarding the impact of the plugin code on speed of message display ... 

reproducible as far back as Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072500 Shredder/3.0a2pre. I did not test v2, nor go back further on v3, nor attempt to determine if there is any change in behavior over time between then and now.  

It disturbs me that I can't account for why I was happy with v3 performance until a couple months ago.

note: 
* arrow key navigation is subject to an imposed delay set in mailnews.threadpane_select_delay https://bugzilla.mozilla.org/show_bug.cgi?id=242791#c15
* (non-arrow) keyboard navigation and direct click on messages should have instant display
* bugs: there are plenty of possible dupes in query [1]. including bugs from v2 if v2 is found to have the same problem. the oldest "v3" are
 Bug 533499 - When quickly cycling through messages with [, ], (go forward, and go back in history) message preview pane can be stale
 Bug 534823 - Delay when attempting to read mail with intermittent "(not responding)" and gloda indexing turned off
* the reporter of bug 534823 indicates plugin.scan.plid.all = false solves his problem, as does Robert in http://getsatisfaction.com/mozilla_messaging/topics/slow_unresponsive#reply_3542355

[1] slow msg display ... https://bugzilla.mozilla.org/buglist.cgi?type1-0-0=anywordssubstr&type0-1-0=nowordssubstr&order=Bug%20Number&field0-1-0=short_desc&short_desc=message%20mail&field0-0-0=short_desc&bug_severity=major&bug_severity=normal&bug_severity=minor&type1-0-1=substring&value1-0-1=perf&resolution=---&resolution=DUPLICATE&resolution=INCOMPLETE&classification=Client%20Software&classification=Components&chfieldto=Now&query_format=advanced&value0-2-0=address%20filter%20compos%20&value0-1-0=address%20filter%20compos%20delet%20memory%20larg%20usb%20processing%20contact%20reply%20scroll%20check%20alert%20downloa%20compact%20Manager%20send%20script%20search%20drag%20mark%20html&chfieldfrom=2007-01-01&value1-0-0=slow%20perform%20delay%20pause&field0-2-0=component&short_desc_type=anywordssubstr&type0-0-0=nowords&value0-0-0=count%20counts%20&type0-2-0=nowordssubstr&field1-0-0=short_desc&product=Thunderbird&field1-0-1=keywords
(In reply to comment #1)
> ... or, we might implement our own doc viewer factory:
> <http://mxr.mozilla.org/comm-central/source/mozilla/webshell/public/nsIDocumentLoaderFactory.idl>
> and do the stream conversion there, if possible.

AFACIT nsDirectoryViewerFactory is a semi-reasonable example of this.
Attached patch patch, v1 (obsolete) — Splinter Review
Right, this adds a wrapper DLF. I have no idea how to test this, but Mozmill passed with this patch applied.

I've kicked off try server builds.
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
Attachment #478236 - Flags: superreview?(bienvenu)
Attachment #478236 - Flags: review?(bugzilla)
A libxul build apparently doesn't touch mailnews/base/build at all, so I've moved the files to mailnews/base/src
Attachment #478236 - Attachment is obsolete: true
Attachment #478253 - Flags: superreview?(bienvenu)
Attachment #478253 - Flags: review?(bugzilla)
Attachment #478236 - Flags: superreview?(bienvenu)
Attachment #478236 - Flags: review?(bugzilla)
Try server builds are available at http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/sid.bugzilla@gmail.com-abd26e342cd7/ -- Wayne, could you try them out?
(In reply to comment #6)
> Try server builds are available at
> http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/sid.bugzilla@gmail.com-abd26e342cd7/
> -- Wayne, could you try them out?

looks great so far - properly meets the performance expectations as it *should* work per comment 2.  will run it for the next few days.
Comment on attachment 478253 [details] [diff] [review]
patch v1.1 [checkin: comment 19]

Could use ? operator and get rid of duplicate calls.

+  if (viewSource) {
+    rv = factory->CreateInstance("view-source", aChannel, aLoadGroup,
+                                 TEXT_HTML "; x-view-type=view-source",
+                                 aContainer, aExtraInfo, getter_AddRefs(listener),
+                                 aDocViewer);
+  } else {
+    rv = factory->CreateInstance("view", aChannel, aLoadGroup, TEXT_HTML,
+                                 aContainer, aExtraInfo, getter_AddRefs(listener),
+                                 aDocViewer);
Attachment #478253 - Flags: superreview?(bienvenu) → superreview+
(In reply to comment #8)
> Comment on attachment 478253 [details] [diff] [review]
> patch v1.1
> 
> Could use ? operator and get rid of duplicate calls.
> 
> +  if (viewSource) {
> +    rv = factory->CreateInstance("view-source", aChannel, aLoadGroup,
> +                                 TEXT_HTML "; x-view-type=view-source",
> +                                 aContainer, aExtraInfo,
> getter_AddRefs(listener),
> +                                 aDocViewer);
> +  } else {
> +    rv = factory->CreateInstance("view", aChannel, aLoadGroup, TEXT_HTML,
> +                                 aContainer, aExtraInfo,
> getter_AddRefs(listener),
> +                                 aDocViewer);

There are two arguments that are different, so ?: will have to be used twice. I'd rather keep this...
(In reply to comment #12)

> There are two arguments that are different, so ?: will have to be used twice.
> I'd rather keep this...

Ah, so there are. nm, then.
FTR, plugin.disable didn't help me
Comment on attachment 478253 [details] [diff] [review]
patch v1.1 [checkin: comment 19]

I've had a think about this and I can't see any issues with it. Also a=Standard8 for 1.9.2 branch, please land in both places so we can maximise testing before the next release builds start.
Attachment #478253 - Flags: review?(bugzilla)
Attachment #478253 - Flags: review+
Attachment #478253 - Flags: approval-thunderbird3.1.5+
Comment on attachment 478253 [details] [diff] [review]
patch v1.1 [checkin: comment 19]

Sid tells me he's got some concerns that this is right, and hence we should really bake this for a while on trunk.
Attachment #478253 - Flags: approval-thunderbird3.1.5+ → approval-thunderbird3.1.5-
(In reply to comment #16)
> Comment on attachment 478253 [details] [diff] [review]
> patch v1.1
> 
> Sid tells me he's got some concerns that this is right, and hence we should
> really bake this for a while on trunk.

The concern I have is that we used to convert from message/rfc822 to */*, but now we always convert from message/rfc822 to text/html. This seems right looking at <http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsStreamConverter.cpp#354>, but I don't know for sure.
As discussed on IRC, here's a patch that disables plugins in Thunderbird. This should only land on 1.9.2.
Attachment #479397 - Flags: review?(bugzilla)
Attachment #479397 - Flags: approval-thunderbird3.1.5?
blocking-thunderbird3.1: --- → ?
http://hg.mozilla.org/comm-central/rev/e19b069946a4
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
Attachment #478253 - Attachment description: patch v1.1 → patch v1.1 [checkin: comment 19]
(In reply to comment #18)
> Created attachment 479397 [details] [diff] [review]
> Disable plugins on 1.9.2
> 
> As discussed on IRC, here's a patch that disables plugins in Thunderbird. This
> should only land on 1.9.2.

Why was a new pref necessary
mailnews.message_display.allow.plugins is there already.
This will surely confuse users that regularly use plugins.
Just default it to "false" again, that will just annoy them, and not make them irate, looking for the new pref.
(In reply to comment #20)
> (In reply to comment #18)
> > Created attachment 479397 [details] [diff] [review] [details]
> > Disable plugins on 1.9.2
> > 
> > As discussed on IRC, here's a patch that disables plugins in Thunderbird. This
> > should only land on 1.9.2.
> 
> Why was a new pref necessary
> mailnews.message_display.allow.plugins is there already.

They do different things. plugin.disable completely stops loading plugins (e.g. even finding they are present). mailnews.message_display.allow.plugins only prevents add-ons from being activated in content.

This bug has to do with loading/scanning of plugins and not with the content activation part.

> This will surely confuse users that regularly use plugins.
> Just default it to "false" again, that will just annoy them, and not make them
> irate, looking for the new pref.

We'll add a relnote that we're disabling all plugins, whilst I know that may not be loud enough for some people, my suspicion is that only a small proportion of users disable plugins, and I think that performance is a much more important priority.

For trunk versions, we'll of course keep the main patch (and fix any regressions) and then plugins will just be enabled again (plus I now have an idea of how to separate the disabling of plugins depending on if we're in an email message or not).
Comment on attachment 479397 [details] [diff] [review]
Disable plugins on 1.9.2

Like I said earlier, we'll take this because it is an improvement in performance for an issue that has been reported quite a bit and we're not confident in the other fix yet. I'll prep support so that they know as well.
Attachment #479397 - Flags: review?(bugzilla)
Attachment #479397 - Flags: review+
Attachment #479397 - Flags: approval-thunderbird3.1.5?
Attachment #479397 - Flags: approval-thunderbird3.1.5+
blocking-thunderbird3.1: ? → .5+
blocking-thunderbird3.2: ? → ---
Attachment #478253 - Flags: approval-thunderbird3.2a1?
Keywords: perf, regression
Summary: Investigate unnecessary plugin directory scans while loading messages → Investigate unnecessary plugin directory scans while loading messages, causes pauses/slow display during mail navigation or click
Version: unspecified → 3.0
Depends on: 601185
Depends on: 605563
This fix saved me from returning to TB2. Thank you everyone!
Blocks: 617467
Whiteboard: [gssolved][gs]
Attachment #478253 - Flags: approval-thunderbird3.2a1?
You need to log in before you can comment on or make changes to this bug.