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)
Tracking
(blocking-thunderbird3.1 .5+, thunderbird3.1 .5-fixed)
RESOLVED
FIXED
Thunderbird 3.3a1
People
(Reporter: rain1, Assigned: rain1)
References
()
Details
(Keywords: perf, regression, Whiteboard: [gssolved][gs])
Attachments
(2 files, 1 obsolete file)
20.50 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
standard8
:
approval-thunderbird3.1.5-
|
Details | Diff | Splinter Review |
718 bytes,
patch
|
standard8
:
review+
standard8
:
approval-thunderbird3.1.5+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•13 years ago
|
blocking-thunderbird3.2: --- → ?
Assignee | ||
Comment 1•13 years ago
|
||
... 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•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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?
Comment 7•13 years ago
|
||
(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 8•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
FTR, plugin.disable didn't help me
Comment 15•13 years ago
|
||
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 16•13 years ago
|
||
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-
Assignee | ||
Comment 17•13 years ago
|
||
(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.
Assignee | ||
Comment 18•13 years ago
|
||
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?
Updated•13 years ago
|
blocking-thunderbird3.1: --- → ?
Assignee | ||
Comment 19•13 years ago
|
||
http://hg.mozilla.org/comm-central/rev/e19b069946a4
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
Assignee | ||
Updated•13 years ago
|
Attachment #478253 -
Attachment description: patch v1.1 → patch v1.1 [checkin: comment 19]
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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+
Updated•13 years ago
|
blocking-thunderbird3.1: ? → .5+
blocking-thunderbird3.2: ? → ---
Updated•13 years ago
|
Attachment #478253 -
Flags: approval-thunderbird3.2a1?
Updated•13 years ago
|
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
Comment 24•13 years ago
|
||
Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/29984cb8fd38
status-thunderbird3.1:
--- → .5-fixed
Comment 26•13 years ago
|
||
This fix saved me from returning to TB2. Thank you everyone!
Updated•13 years ago
|
Whiteboard: [gssolved][gs]
Updated•12 years ago
|
Attachment #478253 -
Flags: approval-thunderbird3.2a1?
You need to log in
before you can comment on or make changes to this bug.
Description
•