413529 - Fix duplicate attachment icons and crashes with RWS

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Peter Weilbacher]

The RWS method to access icons was enabled in bug 411332 only for Firefox. In SeaMonkey and Thunderbird it can cause crashes. This happens when one displays an email with one or more attachments. Instead of one line/icon per attachment two can be displayed and when accessing those, the app crashes.

As far as I remember debugging this problem some time ago, it happens like this:
- Mozilla then asks the system through RWS for an icon
- the system is too slow to respond
- in the meantime another event is triggered somewhere in Gecko to ask
  for the same icon again
- now the system has provided the icon and it gets displayed twice
- one of them is fake, i.e. not associated to the underlying data, so
  when accessing that, it gets a NULL pointer somewhere
I have more detailed debugging notes somewhere but don't find them right now.

Comment 1

[Walter Meinl]

I found something, when you look for seamonkey at
http://mxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgHdrViewOverlay.js#1267
and TB
http://mxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#1348
you'll see
listitem.setAttribute('image', "moz-icon:" + "//" + ...
the patch in bug305061 introduced
http://mxr.mozilla.org/seamonkey/source/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp#618 
nsCAutoString iconURLSpec(NS_LITERAL_CSTRING("moz-icon://"))
Thus, it seems that adding the double slash "//" to moz-icon in mail and mailnews could trigger the delay.
I tested it by manually adding the double slash directly to moz-icon in that file and activated RWS on suite. Seamonkey mailnews shows only one icon per attachment.
I wonder still what would be correct changing the mail/mailnews could or the line in nsMIMEInfoOS2.cpp
Two additional files in mail/mailnews contain also moz-icon: w/o slashes
http://mxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/MsgComposeCommands.js#2599
and
http://mxr.mozilla.org/seamonkey/source/mail/components/compose/content/MsgComposeCommands.js#2675
in case icons for attachments in composition make also trouble

Comment 2

[Walter Meinl]

Sorry, forget comment #1. I've now built TB and SM with RWS enabled in nsIconChannel.cpp, with the original msgHdrOverlay.js and nsMIMEInfoOS2.cpp as in the tree. Surprisingly I don't crash anymore in TB. In Seamonkey it's a bit different. I have two distinct old mails with attachments (jpg images). When I click on them in the list before looking at another mail I see the doubled icons and crash. When I first look at another mail with attachment (so far any other with attachment, to be sure in another mailbox), and then return to the "crashing" mails, I see only one attachment and don't crash. Having access to the same mailboxes with thunderbird those "crashers" have always only one icon displayed and don't crash regardless if I look at them as the very first or subsequently after another.

Comment 3

[Rich Walsh]

Since I can no longer debug this myself, I've created a package to help others figure this out.  It includes a debug version of rwscli08.dll that displays console messages as RWS does its thing.  It also includes "WhatToDebug.txt" which describes how to use the package, and where to set useful breakpoints in a debugger.  You can get it from: http://e-vertise.com/rws/rwsdebug.zip

"For the record", here's a description of the problem:

If nsRwsService doesn't have the icon for some file extension in its cache, it asks RWS to get it.  RWS posts a msg to the WPS, then enters a msg loop and waits for a reply.  When that's received, it exits the loop & returns to the caller.  While in the loop, RWS dispatches msgs to other windows to avoid blocking the queue.

Apparently, one of the msgs RWS dispatches enables a mozilla event to be handled prematurely, causing the phantom attachment to appear.  The questions to be answered are:  1) what is this event?  2) is it needed?  3) is it being handled correctly?

Note:  The problem doesn't occur the second time you open an email because nsRwsService has the icon in its cache, so it doesn't have to call RWS.

Comment 4

[Peter Weilbacher]

Rich, thanks for the debug package which I downloaded and briefly looked at. I had always assumed that it was a problem somewhere deep down in Mozilla's event handling code. But if it is in [1] the RWS support code then this should be much easier to track down and fix. But I'll postpone further investigation until after the FF3 release because FF isn't affected.

[1] I doubt I have ever written four two letter words starting with the same letter in a row, looks weird.

Comment 5

[Rich Walsh]

(In reply to comment #4)
> I had always assumed that it was a problem somewhere deep down in Mozilla's
> event handling code. But if it is in the RWS support code then this should
> be much easier to track down and fix.

I wouldn't describe it like that at all - it _is_ an event handling issue.  The bug is masked when an icon is retrieved synchronously but revealed when it's retrieved asynchronously.  It may be that the event is being handled prematurely or that it's totally redundant (I hope it's the latter).

The debug package is intended to make it very easy to isolate this event.  If you set the breakpoints I recommended, your debugger should only stop once:  when the event in question appears.  If you'll build me a debug version of TB with RWS enabled, I'll trace it.

Comment 6

[Walter Meinl]

Revisiting this bug after almost a year:
a lot of changes happened particularly for the suite code. So, I tried again to enable RWS for seamonkey and went through my mails that triggered the crashes one year ago. I could open any mail with any number of attached files, the number was always correct, no attachment was displayed in duplicate and seamonkey is still alive ;-). Thunderbird seems to be as well stable. Maybe we should consider to enable RWS icon logig also for TB and SM.

Comment 7

[Peter Weilbacher]

Walter: do you now have a machine that is faster now than a year ago? After your comment I wanted to test (and follow Rich's debugging hints), but always forgot...

Comment 8

[Walter Meinl]

(In reply to comment #7)
> Walter: do you now have a machine that is faster now than a year ago? 
No, its the same machine, nothing changed (I'm even back to use not SMP, its still unstable and not as faster than I would wish).
I tested Rich's debug RWS version some time ago, but IIRC I saw similar messages in firefox (that didn't crash) and seamonkey (that did crash sometimes), so I wasn't able to make any sense out of it.
Later I removed the ifdef PHOENIX comment in nsIconChannel and didn't crash in seamonkey (when I opened the same mails that let it crash a year ago). (At that time I had the non-debug RWS reinstalled.) Therefore, I assume the changes in seamonkey code (porting to XUL) did help that it doesn't crash anymore.

Comment 9

[Peter Weilbacher]

Attachment: activate all apps

OK, I have now extensively tested myself with SM from comm-central and mozilla-1.9.1 and indeed I don't see a problem any more. My computer has gotten quite a bit faster over time, but SM from the 1.8.1 branch still crashes. And I haven't seen any complaints in the newsgroup about this.

So I think we are safe to activate the icons for all apps again on m-c and m-1.9.1.

Comment 10

[Walter Meinl]

Comment on attachment 392073 [details] [diff] [review]
activate all apps

sure, we w i l l get feedback on the newsgroup, when there is a machine where it still crashes ;-)

Comment 11

[Peter Weilbacher]

http://hg.mozilla.org/mozilla-central/rev/0b159e56802e

Comment 12

[Peter Weilbacher]

And http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e40397bbe945

1.9.1.2 has branched two days ago, so this will be fixed in 1.9.1.3.