Closed Bug 901959 Opened 13 years ago Closed 13 years ago

Permanent Orange: TEST-UNEXPECTED-FAIL | test_index_messages_imap_offline.js | test_index_messages_imap_online.js | test_index_messages_imap_online_to_offline.js | Error console says ... failure code: 0x80550007 [nsIMsgFolder.getStringProperty]

Categories

(MailNews Core :: Database, defect)

Product:
MailNews Core
Component:
Database
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird24 fixed, thunderbird25 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 26.0
Tracking Flags:
Tracking Status
thunderbird24 --- fixed
thunderbird25 --- fixed

People

(Reporter: standard8, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Do not remove .msf files before folderRenamed notifier V2
1.90 KB, patch
hiro
: review+
standard8
: approval-comm-aurora+
standard8
: approval-comm-beta+
Details | Diff | Splinter Review
Once bug 901514 is fixed, there's then a few more gloda failures of the kind: TEST-INFO | (xpcshell/head.js) | test _async_driver finished (2) WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80500001: file /Users/moztest/comm/main/src/mozilla/intl/uconv/src/nsCharsetConverterManager.cpp, line 300 2013-08-06 16:28:15 gloda.datastore DEBUG !! mapped 34 from imap://user@localhost/gabba27/gabba28 2013-08-06 16:28:15 gloda.index_msg DEBUG folderDeleted notification 2013-08-06 16:28:15 gloda.index_msg INFO Processing deletion of folder gabba28. System JS : ERROR resource:///modules/gloda/datastore.js:2072 Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty] TEST-UNEXPECTED-FAIL | ../../../../resources/logHelper.js | Error console says [stackFrame Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty]] - See following stack: JS frame :: ../../../../resources/logHelper.js :: _errorConsoleTunnel.observe :: line 63 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-INFO | (xpcshell/head.js) | exiting test 2013-08-06 16:28:15 gloda.index_msg DEBUG folderMoveCopy notification (Move: true) System JS : ERROR resource:///modules/gloda/datastore.js:2072 Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty] TEST-INFO | (xpcshell/head.js) | test _async_driver pending (2) TEST-UNEXPECTED-FAIL | ../../../../resources/logHelper.js | Error console says [stackFrame Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty]] - See following stack: JS frame :: ../../../../resources/logHelper.js :: _errorConsoleTunnel.observe :: line 63 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-INFO | (xpcshell/head.js) | exiting test
Attached patch Proposed fix (obsolete) — — Splinter Review
I'm not 100% sure about this. We seem to be receiving a folder deletion, and then a rename for the said folder - however, the folder properties don't exist, as it was deleted, so we blow up. This catches the blowing up, I couldn't spot what was going on with the delete versus rename stuff.
Attachment #786318 - Flags: review?(Pidgeot18)
Summary: Permanent Orange: TEST-UNEXPECTED-FAIL | logHelper.js | Error console says ... failure code: 0x80550007 [nsIMsgFolder.getStringProperty] → Permanent Orange: TEST-UNEXPECTED-FAIL | test_index_messages_imap_offline.js, test_index_messages_imap_online.js, test_index_messages_imap_online_to_offline.js | Error console says ... failure code: 0x80550007 [nsIMsgFolder.getStringProperty]
Keywords: intermittent-failure
Summary: Permanent Orange: TEST-UNEXPECTED-FAIL | test_index_messages_imap_offline.js, test_index_messages_imap_online.js, test_index_messages_imap_online_to_offline.js | Error console says ... failure code: 0x80550007 [nsIMsgFolder.getStringProperty] → Permanent Orange: TEST-UNEXPECTED-FAIL | test_index_messages_imap_offline.js | test_index_messages_imap_online.js | test_index_messages_imap_online_to_offline.js | Error console says ... failure code: 0x80550007 [nsIMsgFolder.getStringProperty]
(In reply to Mark Banner (:standard8) from comment #1) > I'm not 100% sure about this. We seem to be receiving a folder deletion, and > then a rename for the said folder - however, the folder properties don't > exist, as it was deleted, so we blow up. > > This catches the blowing up, I couldn't spot what was going on with the > delete versus rename stuff. Renaming IMAP folder does not preserve .msf file. It just removes old .msf file in nsImapMailFolder::RenameLocal. I do not know why old .msf file is not renamed as well.
Found the reason. bug 56044
My comment in #42 was not problem here. The problem here is nsMsgDBFolder holds old mPath after renaming. In this failure case, the path was /tmp/xpcshell/xpcshellprofile/ImapMail/localhost/gabba28.msf. It has to be /tmp/xpcshell/xpcshellprofile/ImapMail/localhost/gabba27.sbd/gabba28.msf.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #44) > The problem here is nsMsgDBFolder holds old mPath after renaming. > In this failure case, the path was > /tmp/xpcshell/xpcshellprofile/ImapMail/localhost/gabba28.msf. It has to be > /tmp/xpcshell/xpcshellprofile/ImapMail/localhost/gabba27.sbd/gabba28.msf. I was wrong. /tmp/xpcshell/xpcshellprofile/ImapMail/localhost/gabba28.msf is correct here to getStringProperty("indexingPriority"). It is called for old folder. So the real problem is that gloda_ds_renameFolder is called after renaming process has done. As the result, gabba28.msf of old folder was removed.
This patch consists two parts. a) Invoke PropagateDelete() followed by NotifyFolderRenamed() in nsImapMailFolder::RenameClient(). b) Check that oldFolder sits in this._folderByURI in gloda_ds_renameFolder(). About a) PropagateDelete() invokes 'folderDeleted' in gloda/modules/index_msg.js, it ends up removing the folder URI in _folderByURI. These processes have to be done after gloda_ds_renameFolder(). About b) In case of moving folder for IMAP, gloda_ds_renameFolder is called twice. The first one comes from NotifyFolderRenamed() in RenameClient. The second one comes from folderMoveCopyCompleted. The second one should be ignored.
Attachment #790645 - Flags: feedback?(mbanner)
Attachment #790645 - Flags: feedback?(Pidgeot18)
Comment on attachment 786318 [details] [diff] [review] Proposed fix The fact that _mapFolder is exploding suggests that we don't want to be mapping the folder at all. Assuming Hiro's patch makes us not call _mapFolder on the dead URI, I think that's exactly what we want.
Attachment #786318 - Flags: feedback-
Comment on attachment 790645 [details] [diff] [review] Do not remove .msf files before folderRenamed notifier The added gloda guard seems fine and good if it helps us avoid calling _mapFolder on a dead folder and otherwise avoid doing things we don't want to do. My non-binding opinion on moving the C++ line is that we should really add a comment that explains why it should be where it is. Since we're moving it, the position arguably does matter. For example: // Do not propagate the deletion until after we have (synchronously) notified // all listeners about the rename. This allows them to access properties on // the source folder without experiencing failures.
Attachment #790645 - Flags: feedback+
Attachment #786318 - Attachment is obsolete: true
Attachment #786318 - Flags: review?(Pidgeot18)
Comment on attachment 790645 [details] [diff] [review] Do not remove .msf files before folderRenamed notifier Thanks to Andrew for his comments. Given them, I'm definitely happy with this as well, so r=Standard8 with the comment added.
Attachment #790645 - Flags: feedback?(mbanner)
Attachment #790645 - Flags: feedback?(Pidgeot18)
Attachment #790645 - Flags: feedback+
Comment on attachment 790645 [details] [diff] [review] Do not remove .msf files before folderRenamed notifier [Triage Comment] The completed patch has a=Standard8 to land on aurora and beta as well as we need it there.
Attachment #790645 - Flags: approval-comm-beta+
Attachment #790645 - Flags: approval-comm-aurora+
Carrying over review+. Add the comment in comment 52.
Assignee: mbanner → hiikezoe
Attachment #790645 - Attachment is obsolete: true
Attachment #791078 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b55f70dce418
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Comment on attachment 791078 [details] [diff] [review] Do not remove .msf files before folderRenamed notifier V2 [Triage Comment] We need this on aurora/beta to fix unit test bustage. a=Standard8
Attachment #791078 - Flags: approval-comm-beta+
Attachment #791078 - Flags: approval-comm-aurora+
Depends on: 916095
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: