GNOME Bugzilla – Bug 608379
Fix mail fetching with exchange 2010
Last modified: 2010-02-18 18:33:10 UTC
I had 9 messages in my inbox, but evolution-mapi would only display one of them. Attached patch seems to fix this issue. It seems like an obviously safe fix, but I don't know whether it should really be necessary or not. I would also appreciate it if somebody could test that it doesn't cause regressions with different versions of exchange, just to be absolutely sure.
Created attachment 152521 [details] [review] Fix mail fetching with exchange 2010 When testing against exchange 2010, evolution-mapi was displaying the first mail in my inbox but was failing to download any further mails. The root cause of the bug can be illustrated by the following pseudo-code: rowset = QueryRows; struct SPropTagArray tags = set_SPropTagArray(...); for (i = 0 to rowset.numRows) { msg = OpenMessage(rowset[i]); GetProps (msg, tags,..) } so we re-use the 'tags' variable between calls to GetProps(). But the problem is that it seems that GetProps() modifies 'tags', so subsequent calls will fail. Creating a copy of 'tags' for each loop iteration fixes the problem and allows me to see all messages in my inbox.
Comment on attachment 152521 [details] [review] Fix mail fetching with exchange 2010 Can we use talloc_memdup instead of copy_tag_array ? Good to commit. Green light. Thanks.
I could use talloc_memdup, but I would still have to allocate the SPropTagArray struct and then talloc_memdup the aulPropTag array, so I'd be inclined to keep the copy_tag_array convenience function.
Created attachment 152749 [details] [review] Fix mail fetching with exchange 2010 When testing against exchange 2010, evolution-mapi was displaying the first mail in my inbox but was failing to download any further mails. The root cause of the bug can be illustrated by the following pseudo-code: rowset = QueryRows; struct SPropTagArray tags = set_SPropTagArray(...); for (i = 0 to rowset.numRows) { msg = OpenMessage(rowset[i]); GetProps (msg, tags,..) } so we re-use the 'tags' variable between calls to GetProps(). But the problem is that it seems that GetProps() modifies 'tags', so subsequent calls will fail. Creating a copy of 'tags' for each loop iteration fixes the problem and allows me to see all messages in my inbox.
Comment on attachment 152521 [details] [review] Fix mail fetching with exchange 2010 I attached the new patch that I have pushed to git that uses talloc_memdup
re-opening after discussion with jkerihuel on irc. better patch coming soon.
Created attachment 152777 [details] [review] Properly fix fetching with exchange 2010 After discussing this issue with jkerihuel on IRC, we determined that my previous patch was really just a workaround and the real problem was improper allocation of the GetPropsTagArray variable. We should hardly ever need to use talloc directly and here we were using it incorrectly (which, in addition to improper operation, also resulted in a memory leak). This patch essentially reverts my previous 'fix' and implements things properly. With this patch, things still work with exchange 2010, but the patch is much cleaner and more correct. Excerpts from the IRC conversation: <kerihuel> using talloc incorrectly can sometimes lead to unpredictable behavior which sounds like what you've experienced <kerihuel> it only got fixed when you've started to overwrite the existing array ... <kerihuel> jonner: line 1170, then line 1245. You allocate the SPropTagArray with mem_ctx, then the sub elements with the same parent context <kerihuel> which means that when you call MAPIFreeBuffer over GetPropsArray, none of the ulPropTag you allocated memory for will be released
thanks, pushed.
I reverted both your patches (commit e27c4d5), it was causing crashers in eds. Note for your commit log: you didn't notice, so telling you that evolution team is mostly following gtk+ style of commit log, which in this particular case means that if you are fixing something with a bug, then you are not adding there a bug url, but in the short summary write its number in one of the form: Bug #123456 - <your short description here> Bug 123456 - <your short description here> see examples here, http://git.gnome.org/browse/evolution-data-server/log/ where is also used "(bnc)" to indicate that the bug number is from other bugzilla. Thanks for your understanding.
can you be more specific about what crashers you were seeing?
Created attachment 153016 [details] [review] Re-Fix mail fetching with exchange 2010 When testing against exchange 2010, evolution-mapi was displaying the first mail in my inbox but was failing to download any further mails. The root cause of the bug can be illustrated by the following pseudo-code: rowset = QueryRows; struct SPropTagArray tags = set_SPropTagArray(...); for (i = 0 to rowset.numRows) { msg = OpenMessage(rowset[i]); GetProps (msg, tags,..) } so we re-use the 'tags' variable between calls to GetProps(). But the problem is that it seems that GetProps() modifies 'tags', so subsequent calls will fail. Creating a copy of 'tags' for each loop iteration fixes the problem and allows me to see all messages in my inbox.
OK, this patch should hopefully finally be complete and correct. testing appreciated.
Thanks, patch works good. Please commit to master. (In reply to comment #10) > can you be more specific about what crashers you were seeing? Ouch, of course I should, I thought you'll see the one from Akhil on IRC, thus didn't give much attention to the proper process for this. I'm sorry for that.
Patch has been committed in master so i will close the bug. http://git.gnome.org/browse/evolution-mapi/commit/?id=d1e97af660983a891c715c3bac9bdf97676c7941
*** Bug 573725 has been marked as a duplicate of this bug. ***