623522 - Stop IMAP touching the preference service from background threads | Permanent Orange | TEST-UNEXPECTED-FAIL | test_dod.js | false == true

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Mark Banner (:standard8)]

Since bug 619487 landed (note the check-in comment was wrong), we've been seeing test failures in test_dod.js:

EST-UNEXPECTED-FAIL | /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/test_dod.js | false == true - See following stack:
JS frame :: /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/head.js :: do_throw :: line 439
JS frame :: /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/head.js :: do_check_eq :: line 491
JS frame :: /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/head.js :: do_check_true :: line 503
JS frame :: /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/test_dod.js :: run_test :: line 90
JS frame :: /builds/slave/comm-central-trunk-macosx64-opt-unittest-xpcshell/build/xpcshell/head.js :: _execute_test :: line 322

In a debug build it shows up that we're accessing the preferences service from a background thread which is no longer allowed.

Comment 1

[Shawn Wilsher :sdwilsh]

To be clear, it was never safe to access.  We just didn't warn about it.

Comment 2

[Mark Banner (:standard8)]

I've got half a patch for this so far.

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix

This patch needs the patch on bug 623685 for it to apply correctly.

This changes the code so that the preference values for the trash folder name and mailnews.display.prefer_plaintext are obtained in nsImapProtocol::SetupWithUrl.

I've removed nsImapProtocol::GetTrashFolderName as with the change it just makes sense to inline the stored variable.

The prefer plaintext preference gets stored in nsImapProtocol and is obtained when the various body parts are created.

I had a few issues with the unit test there as the change in pref location was tending to mean that the value wouldn't get re-set (because of the cached body parts I think), and so I ended up rewriting the test and selecting a different folder in-between. That seems to make it stable on my mac at least.

I also switched the test from DOS line endings to unix, given that most of it was a rewrite anyway, I don't think the diffs would be all that useful.

Comment 4

[David :Bienvenu]

I think the unit test issues you saw might have a different cause, but I need to do some more investigation. The old tests pass for me, but from looking at a protocol log,  I noticed that other than the first message test, none of the other messages tests actually invoke the imap code, because we download the folder for offline use, and don't get into the imap body part code at all, because of the code I wrote to avoid fetching the same message from the server if we've already fetched it.

I'd like to avoid having to switch folders in the test, since that seems rather hacky. It might be the caching of shell parts, but afaict, we don't clear the cache when you switch folders.

Comment 5

[David :Bienvenu]

Turning off autosync of offline stores makes the old test fail reliably for me. I'll check if doing that with the new version of the test makes it fail as well. 

I think the test failure is showing a real bug that users would see, and we should throw away the shell cache if prefer_plaintext pref changes. I'll try that out, and attach a tweaked version of the patch if it works out.

Comment 6

[David :Bienvenu]

Another possibility would be to just to stick the current value of the pref in the host session list, instead of remembering it in each body part. The host session list is meant to be used for thread safe storage.

Comment 7

[David :Bienvenu]

Attachment: invalidate body shell cache when plaintext pref changes

this patch has several changes from the previous one:

reverts dod test changes, and disables autosync for dod test
adds a host session list method to clear the body shell cache
clears the body shell cache for a host if the prefer plain text pref changes
removes a debugging printf

Mark, do you want to try this out and see if the tests pass for you? You might want to refix the line endings for the test...

If this patch works for you, consider your parts of it r=me.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 502043 [details] [diff] [review]
invalidate body shell cache when plaintext pref changes

r=Standard8 on your parts.

Comment 9

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/a36b59e34e7e