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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Rain [:rain1] (inactive)]

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?

Comment 1

[Rain [:rain1] (inactive)]

... 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.

Comment 2

[Wayne Mery (:wsmwk)]

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

Comment 3

[Mark Banner (:standard8)]

(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.

Comment 4

[Rain [:rain1] (inactive)]

Attachment: patch, v1

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.

Comment 5

[Rain [:rain1] (inactive)]

Attachment: patch v1.1 [checkin: comment 19]

A libxul build apparently doesn't touch mailnews/base/build at all, so I've moved the files to mailnews/base/src

Comment 6

[Rain [:rain1] (inactive)]

Try server builds are available at http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-abd26e342cd7/ -- Wayne, could you try them out?

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> Try server builds are available at
> http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-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 8

[David :Bienvenu]

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);

Comment 12

[Rain [:rain1] (inactive)]

(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...

Comment 13

[David :Bienvenu]

(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.

Comment 14

[Wayne Mery (:wsmwk)]

FTR, plugin.disable didn't help me

Comment 15

[Mark Banner (:standard8)]

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.

Comment 16

[Mark Banner (:standard8)]

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.

Comment 17

[Rain [:rain1] (inactive)]

(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.

Comment 18

[Rain [:rain1] (inactive)]

Attachment: 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.

Comment 19

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/e19b069946a4

Comment 20

[Joe Sabash [:JoeS1]]

(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.

Comment 21

[Mark Banner (:standard8)]

(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 22

[Mark Banner (:standard8)]

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.

Comment 24

[Mark Banner (:standard8)]

Checked into 1.9.2:

http://hg.mozilla.org/releases/comm-1.9.2/rev/29984cb8fd38

Comment 26

[Val]

This fix saved me from returning to TB2. Thank you everyone!