The default bug view has changed. See this FAQ.

don't need event target (thread) when running imap urls

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 12.0
x86_64
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Summary: dpm → don't need event target (thread) when running imap urls
(Assignee)

Comment 1

5 years ago
When we got rid of xpcom proxies, we could have also gotten rid of the event target stuff. This had a rather large ripple effect, but it gets rid of an unneeded parameter/member variable in a bunch of places.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 576374 [details] [diff] [review]
proposed fix

it's a rather large patch, but very simple.
Attachment #576374 - Flags: superreview?(mbanner)
Attachment #576374 - Flags: review?(neil)

Comment 3

5 years ago
Comment on attachment 576374 [details] [diff] [review]
proposed fix

Haven't run any tests, but it opened my Inbox OK.

>+    rv = imapService->DiscoverChildren( this, this, m_onlineFolderName, nsnull);
Nit: extraneous space after (

> {
>+
>+  return ChangeFolderSubscription(aFolder, aFolderName, 
>                                   "/subscribe>", urlListener, url);
Nit: extraneous blank line after {
Attachment #576374 - Flags: review?(neil) → review+
(Assignee)

Comment 4

5 years ago
pinging for sr; I'd like to land this relatively soon, to avoid conflicts...
Comment on attachment 576374 [details] [diff] [review]
proposed fix

Review of attachment 576374 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but for some reason, I'm seeing lots of failures in debug mode, specifically in:

mailnews/base/test/unit/test_imapPump.js
mailnews/imap/test/unit/test_filterCustomHeaders.js
mailnews/imap/test/unit/test_imapMove.js
mailnews/imap/test/unit/test_preserveDataOnMove.js
mailnews/imap/test/unit/test_saveImapDraft.js
mailnews/imap/test/unit/test_saveTemplate.js

These don't seem to happen without the patch applied. Although I'm a bit confused as functionally you don't seem to be changing that much.

It looks like it is generally the shutdown of the test that is the issue.

::: mailnews/local/src/nsLocalUndoTxn.cpp
@@ +157,5 @@
> +    // folder so use lite select to do the trick
> +    rv = imapService->LiteSelectFolder(folder,
> +                                       urlListener, nsnull, nsnull);
> +    if (!deleteFlag)
> +        rv =imapService->AddMessageFlags(folder,

nit: needs a space after equals.
Attachment #576374 - Flags: superreview?(mbanner) → superreview-
(Assignee)

Comment 6

5 years ago
hmm, I guess that's what I get for running the tests in release mode. I'll look into it.
(Assignee)

Comment 7

5 years ago
tests are all working fine here - I'll try try server.
(Assignee)

Comment 8

5 years ago
try server worked fine, but try server doesn't finish debug builds, so it wasn't so helpful. I still can't reproduce any test failures, and since I'm essentially only removing an unused paramter, it's hard to believe that the test failures are caused by my patch.
(Assignee)

Comment 9

5 years ago
OK, I do see those test failures on mac. Suspiciously, I think most if not all of the failing tests use IMAPPump.js
(Assignee)

Comment 10

5 years ago
Created attachment 584359 [details] [diff] [review]
de-bitrotted patch

this applies on the trunk, and no longer do I see the test failures on debug mac builds, so I suspect the pluggable store landing changed some timing issues. I'll request a try server build with this patch.
Attachment #576374 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
The test failures seem to be race conditions. test_imapPump.js needs to turn off autosync, or an autosync can start during shutdown, which leads to an assertion. test_filterCustomHeaders has a mark read filter, which starts a url that runs during shutdown, again causing an assertion. I suspect the other tests have similar issues.
(Assignee)

Comment 12

5 years ago
Created attachment 587157 [details] [diff] [review]
fix for unit tests

This fixes all the unit tests for me, using several approaches:

1. Make sure auto sync is turned off for tests
2. Abort queued urls at fake server shutdown.
3. Don't try to run queued urls if we're offline
4. For tests that run "extra" urls, do a performTest() on the expected command

I'll generate a try server build with this patch, though I'm not having a lot of luck getting the try server to report actual tests results at the moment (times out)
Attachment #584359 - Attachment is obsolete: true
Attachment #587157 - Flags: review?(mbanner)
(Assignee)

Comment 13

5 years ago
try server request - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-0331ada9e01e
Comment on attachment 587157 [details] [diff] [review]
fix for unit tests

Review of attachment 587157 [details] [diff] [review]:
-----------------------------------------------------------------

Neil already gave r, so I'll make this sr ;-)
Attachment #587157 - Flags: review?(mbanner) → superreview+
(Assignee)

Comment 15

5 years ago
fixed - http://hg.mozilla.org/comm-central/rev/25d62c5e3121
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
You need to log in before you can comment on or make changes to this bug.