692735 - Crash exporting messages in HTML format with ImportExportTools

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[intendentedelleacque]

Attachment: ImportExportTools-2.6.2.2.xpi

User Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9.2.21) Gecko/20110830 Firefox/3.6.21
Build ID: 20110830092825

Steps to reproduce:

I'm the developer of ImportExportTools extension.
Many users wrote to me, about a crash they get when trying to export messages in HTML format.
The crash happens after a random number of messages and just with Thunderbird 5 or higher.
All my reports come from Windows or MAC users and I can't reproduce this bug in any way on my Linux machine.

Crash logs (I've just few) have all many references to  "%SystemRoot%\system32\mswsock.dll" , you can find it a couple here:

http://services.gxware.org/nopaste/?3673
http://services.gxware.org/nopaste/?3674

All crash reports I got, seem to fall into this crash: 

https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=XPC_WN_NoHelper_Finalize&reason_type=contains&date=10%2F07%2F2011%2001%3A14%3A11&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=XPC_WN_NoHelper_Finalize

All users report that crash happens just exporting in HTML/plain-text format, but not in EML format.
The code for export in HTML is very similar to the code for export in EML.

The only difference is that EML export is done just through with a simple call to 

messengerService.streamMessage(msguri, myEMLlistner, msgWindow, null, false, null);

while HTML export is performed with

var nschannel =  Components.classes["@mozilla.org/network/input-stream-channel;1"]
   	  .createInstance(Components.interfaces.nsIInputStreamChannel);
nschannel.setURI(nsURI);
var streamConverterService = Components.classes["@mozilla.org/streamConverters;1"]
   	   	  .getService(Components.interfaces.nsIStreamConverterService);
var streamListner=streamConverterService.asyncConvertData("message/rfc822", "text/html", myTxtListener, nschannel);
messageService.streamMessage(uri, streamListner, msgWindow, null, false, null);		

The rest of the code is pretty identical, so I guess that the crash is caused by nschannel.setURI(nsURI) or by the asyncConverter.

Notice that it seems have a close relation with https://bugzilla.mozilla.org/show_bug.cgi?id=682916, so this crash could be a symptom of a very frequent problem, present also on Firefox.

Steps to reproduce:

- install ImportExportTools extension, version 2.6.2.2 (attached here)
- try to export a large number of messages in HTML format, with Thunderbird 5 or higher
- after a random number of messages, sometimes you'll get a crash of Thunderbird

I'm contact with some users that can help us and me to debug this problem, so tell me which tests can be useful.

Comment 1

[:aceman]

Can you post any of your crash IDs?

If you are the developer of the extension, you have identified, that it crashes when calling those Firefox methods below?

Comment 2

[intendentedelleacque]

(In reply to aceman from comment #1)
> Can you post any of your crash IDs?

Crash IDs of some users (with different machine and different TB versions):

http://crash-stats.mozilla.com/report/index/bp-b5cac55c-200b-47bd-a3ef-f8a7f2110813
http://crash-stats.mozilla.com/report/index/bp-d11a36b5-67d7-4cfb-a696-ec51b2110813
http://crash-stats.mozilla.com/report/index/bp-f276af73-2abe-47f0-a16d-ada232110813
http://crash-stats.mozilla.com/report/index/bp-5e32450f-d162-4f4b-9af1-805a62111006
http://crash-stats.mozilla.com/report/index/bp-c9f4bab7-b82b-44ba-90a3-510ed2111006
http://crash-stats.mozilla.com/report/index/bp-c4d2e762-1b3f-4157-b81d-a92902111007
http://crash-stats.mozilla.com/report/index/bp-6ea9d42f-465f-4e63-8c48-09ec32111007


> If you are the developer of the extension, you have identified, that it
> crashes when calling those Firefox methods below?

I don't understand this: the code above is Thunderbird specific and so I can't use it on Firefox.
But some of the crashes above seem to have relations with other Firefox crashes.

Comment 3

[Ludovic Hirlimann [:Usul]]

Hum the crashes are a bit different so we'll go for the one you describe in comment 0

Comment 4

[intendentedelleacque]

Other crash IDs:

https://crash-stats.mozilla.com/report/index/bp-3282fb24-a751-4173-991f-c89662111007
https://crash-stats.mozilla.com/report/index/bp-187e28f0-5e7d-4f72-b873-f386b2111007
https://crash-stats.mozilla.com/report/index/bp-4a6fb868-5bf2-47c6-b8fc-0ff192111007
https://crash-stats.mozilla.com/report/index/bp-70dd05bb-d330-43f8-b0dd-e332a2111007
https://crash-stats.mozilla.com/report/index/bp-35d45b8f-151c-4137-a461-816fe2111006
https://crash-stats.mozilla.com/report/index/bp-c59fbad5-1848-4229-8ee0-1b5cd2111006

Yes signatures with TB7 seem to be different from TB5 ones.

Comment 5

[:aceman]

Sorry, that was a typo, I meant Thunderbird instead of Firefox :)

Comment 6

[intendentedelleacque]

Update: with the help of some helpful italian users, I have found a workaround against this bug.

The 2.6.3 version of my extension now uses just messageService.streamMessage with different parameters, to produce the HTML output (off-topic --> why messageService.streamMessage acts differently with IMAP and POP3 messages???).

This workaround confirms that the crashes are caused by  the nsIChannel or the asyncConverter.
Basically my extension did this:

- create an array with a number of message URIs
- call the first item of the array, create the nsIChannel and the asyncConverter
- call to messageService.streamMessage
- on stop loading it did some actions and then iterate again the cycle with the next array item

I hope that someone more skilled than me can investigate about this, because it could be important for other crash cases.
And moreover the HTML code generated by asyncConverter seem cleaner.

Comment 7

[:aceman]

Would you be able to create a test extension that just runs those steps to the crash (best activated by some button)? I could then test it and confirm the bug, without studying how to operate the ImportExportTools extension.

Comment 8

[intendentedelleacque]

(In reply to aceman from comment #7)
> Would you be able to create a test extension that just runs those steps to
> the crash (best activated by some button)? I could then test it and confirm
> the bug, without studying how to operate the ImportExportTools extension.

I've created an extension with a minimal code, that causes the crash.
I've uploaded here, it's called bugtest.xpi
Steps to reproduce the crash:

1) install it on Thunderbird 5 or higher  Windows version
2) right-click on a folder (IMAP or POP3 or local seem to be the same) with a quite big amount of messages, 300 or more
3) click on "Test for bug 692735"
4) after few seconds you will see on status bar a number that is the total of messages in the folder. This means that all went ok
5) perform step 4 for 3 or 4 times. You could already get a crash after a random number of messages.
6) select another folder and perform again step 4
7) after some times you performed steps 5 and 6 you'll get probably a crash

I've tried on Windows XP and Thunderbird 7 and following these steps I get a crash after a while.
The same code on the same machine with Linux didn't cause any crash.
Sometimes the crash happens when running the code, sometimes when i select another folder after having run it.
Creating this minimal extension, I realized that the crash is caused when streaming a message into the listener created with async converter.

Comment 9

[intendentedelleacque]

Attachment: Mininal test extension the causes the crash

Comment 10

[:aceman]

Great, thank you. I do not have TB on Windows, but will at least try it on Linux.

Comment 11

[:aceman]

It crashed for me just fine at 3rd run, (2 on 532 message folder, 3rd on 842 one), TB 8 beta, linux 32bit.
Crash ID: bp-3273ce69-3a13-4b17-9804-9d1242111011

Comment 12

[:aceman]

For some reason there is a different signature?
[e-mail] can you post crash ID from a crash made with this extension on your machine?

Comment 13

[David :Bienvenu]

I suspect that stack is actually correct, and the issue might have to do with a ref-counting issue on the msd->channel.

Comment 14

[intendentedelleacque]

Here are two crash IDs with the minimal extension on Windows XP:

https://crash-stats.mozilla.com/report/index/bp-6a6de14f-e266-4993-a7e8-9921a2111012
https://crash-stats.mozilla.com/report/index/bp-b538ea87-152a-4a13-946f-f2c8b2111012

Again two different signatures.
I notice that when Thunderbird enters in unstable state and then crashes, the cycle "for" had a stop before processing all messages.

Comment 15

[Ludovic Hirlimann [:Usul]]

(In reply to David :Bienvenu from comment #13)
> I suspect that stack is actually correct, and the issue might have to do
> with a ref-counting issue on the msd->channel.

Who's code is this ours or the extensions ?

Comment 16

[:aceman]

If the extension is pure JS, aren't all crashes caused by bugs in our code (Gecko)?

Comment 17

[David :Bienvenu]

(In reply to aceman from comment #16)
> If the extension is pure JS, aren't all crashes caused by bugs in our code
> (Gecko)?

highly likely it's a ref-counting issue in our code. Though I haven't tried it yet. It might also be that the extension is somehow aborting the streaming exposing the issue.

Comment 18

[intendentedelleacque]

The "minimal" version of the code doesn't do anything but streaming the message into the listener created by asyncConverter.

As far as I know, this bug is present just on Thunderbird 5 or higher, so it must be related to some change done between 3.1 and 5 version.

This is the complete JS code of minimal extension:

---- BEGIN -----

function runCrashCode() {
	var msgFolder = GetFirstSelectedMsgFolder();
	var msgUriArray = new Array;
	var total = msgFolder.getTotalMessages(false);
	if (total == 0) 
		return;
	for (i=0;i<total;i++) {
		var uri = gDBView.getURIForViewIndex(i);
		var nsURI = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService).newURI(uri, null, null);
		var nschannel =  Components.classes["@mozilla.org/network/input-stream-channel;1"]
      	  		.createInstance(Components.interfaces.nsIInputStreamChannel);
		nschannel.setURI(nsURI);
		var streamConverterService = Components.classes["@mozilla.org/streamConverters;1"]
      	   	  .getService(Components.interfaces.nsIStreamConverterService);
		var streamListner=streamConverterService.asyncConvertData("message/rfc822", "text/html", myTxtListener, nschannel);
		var messageService = messenger.messageServiceFromURI(uri);
		messageService.streamMessage(uri, streamListner, msgWindow, null, false, null);	
		document.getElementById("statusText").setAttribute("label", i);
	}
}

 var myTxtListener = {
	QueryInterface : function(iid)  {
                if (iid.equals(Components.interfaces.nsIStreamListener) ||   
                    iid.equals(Components.interfaces.nsISupports))
                 return this;
        
                throw Components.results.NS_NOINTERFACE;
                return 0;
        },      
		
	onStartRequest: function(request, context) {},
	onDataAvailable: function(request, context, inputStream, offset, count) {},
	onStopRequest: function(request, context, statusCode) {}
};

----- END ----

Comment 19

[David :Bienvenu]

I bet it wouldn't crash if you serialized your streaming; i.e., stream one message, and when you get notified that the first stream has finished, stream the next message. Does the real extension also stream a bunch of messages all at once?

Comment 20

[intendentedelleacque]

(In reply to David :Bienvenu from comment #19)
> I bet it wouldn't crash if you serialized your streaming; i.e., stream one
> message, and when you get notified that the first stream has finished,
> stream the next message. Does the real extension also stream a bunch of
> messages all at once?

No David, the extension serializes the streaming, stream the message one by one and crashes the same.
I've changed the minimal test extension, adding an option "Test for bug 692735 (Serialized)" and I got these 2 crashes the same on WinXP and Thunderbird 7.0.1:

http://crash-stats.mozilla.com/report/index/bp-4c4902f2-7d7e-42b7-9569-efeb32111013

http://crash-stats.mozilla.com/report/index/bp-6237cb8a-9922-47ea-a66f-9083e2111013

Here is the code used in "serialized" minimal version:


----- BEGIN --------

var msgUriArray;
var exportNumber;

function runCrashCodeSerialized() {
	var msgFolder = GetFirstSelectedMsgFolder();
	msgUriArray = new Array;
	var total = msgFolder.getTotalMessages(false);
	if (total == 0) 
		return;
	for (i=0;i<total;i++) 
		msgUriArray[i] = gDBView.getURIForViewIndex(i);
	exportNumber = 0;
	testStreamRun(0);
}

function testStreamRun(index) {
	var uri = msgUriArray[index];
	var nsURI = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService).newURI(uri, null, null);
	var nschannel =  Components.classes["@mozilla.org/network/input-stream-channel;1"]
        .createInstance(Components.interfaces.nsIInputStreamChannel);
	nschannel.setURI(nsURI);
	var streamConverterService = Components.classes["@mozilla.org/streamConverters;1"]
         	  .getService(Components.interfaces.nsIStreamConverterService);
	var streamListner=streamConverterService.asyncConvertData("message/rfc822", "text/html", myTxtListenerSerialized, nschannel);
	var messageService = messenger.messageServiceFromURI(uri);
	messageService.streamMessage(uri, streamListner, msgWindow, null, false, null);	
}

 var myTxtListenerSerialized = {

	QueryInterface : function(iid)  {
	            if (iid.equals(Components.interfaces.nsIStreamListener) ||   
	                  iid.equals(Components.interfaces.nsISupports))
	                return this;
        
	               throw Components.results.NS_NOINTERFACE;
	               return 0;
	   },      
		
	onStartRequest: function(request, context) {},
		
	onDataAvailable: function(request, context, inputStream, offset, count) {},

	onStopRequest: function(request, context, statusCode) {
		exportNumber = exportNumber + 1;
		document.getElementById("statusText").setAttribute("label", exportNumber);
		if (exportNumber < msgUriArray.length)
				testStreamRun(exportNumber);			
	}
};

---- END ------

Comment 21

[intendentedelleacque]

Attachment: Minimal test extension, with "serialized" code option

Comment 22

[intendentedelleacque]

(In reply to intendentedelleacque from comment #20)

> No David, the extension serializes the streaming, stream the message one by
> one and crashes the same.

To clarify: here I mean the "real" extension, ImportExportTools 2.6.2.2 or lower

Comment 23

[David :Bienvenu]

Attachment: proposed fix

protz, if you don't have time to look at this, let me know. Basically, this patch changes a struct into a class, and uses a comptr for nsIChannels so the channel can't be deleted out from under us.

Comment 24

[intendentedelleacque]

David, this means that the cause of the crash is that the channel is deleted while streaming the message?
How can this happen? And why just with Thunderbird 5 or higher?
Forgive my questions, I'm curious and willing to learn :-)

Comment 25

[Ludovic Hirlimann [:Usul]]

Comment on attachment 566897 [details] [diff] [review]
proposed fix

(In reply to David :Bienvenu from comment #23)
> Created attachment 566897 [details] [diff] [review] [diff] [details] [review]
> proposed fix
> 
> protz, if you don't have time to look at this, let me know. Basically, this
> patch changes a struct into a class, and uses a comptr for nsIChannels so
> the channel can't be deleted out from under us.

Comment 26

[Jonathan Protzenko [:protz]]

Heh, no wonder I did not see that comment. I should be able to look into it over the weekend.

Comment 27

[Jonathan Protzenko [:protz]]

Comment on attachment 566897 [details] [diff] [review]
proposed fix

This is pretty basic so you have my green light. Are we positive that other people (who potentially want to free the channel) also use nsCOMPtrs? If other people are using raw pointers and ruthlessly PR_FREE'ing them, that would still be a problem, no?

Comment 28

[David :Bienvenu]

(In reply to Jonathan Protzenko [:protz] from comment #27)
 
> Are we positive that other
> people (who potentially want to free the channel) also use nsCOMPtrs? If
> other people are using raw pointers and ruthlessly PR_FREE'ing them, that
> would still be a problem, no?
libmime is ancient, written before comptrs. In fact, I'm fairly confident it's the oldest code in mailnews. All the other code that uses channels is more recent, and uses comptrs, or understands the ownership model. Otherwise, we'd be a lot more crashy.

Re the question as to why this happens only in TB 5 and later, I don't know. I'm wondering if changing folders destroys the doc shell that was holding onto the channel, though stream message shouldn't be using the display doc shell.

Comment 29

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/142afe5421fb

Comment 30

[Nomis101]

In mimemult.cpp
> mime_stream_data *msd = (smime_stream_data *)body->options->stream_closure;

I think this should be mime_stream_data and not smime_stream_data ?

Comment 31

[David :Bienvenu]

(In reply to Nomis101 from comment #30)
> In mimemult.cpp
> > mime_stream_data *msd = (smime_stream_data *)body->options->stream_closure;
> 
> I think this should be mime_stream_data and not smime_stream_data ?

yes, sorry about that, fix was pushed yesterday - http://hg.mozilla.org/comm-central/rev/c9ba8a4a9e78