Closed Bug 520788 Opened 12 years ago Closed 12 years ago

autoconfig with bogus username/password alerts about "this folder is being processed"

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: philor, Assigned: Bienvenu)

Details

(Keywords: regression, Whiteboard: [no l10n impact])

Attachments

(1 file, 1 obsolete file)

STR:
1. Run autoconfig with goats@harborside.com/goats
2. Click "create"
3. Get an alert

which bienvenu says is coming when we try the second form of username when we aren't yet done trying the first. It's only sort of moderately annoying when you've started from the File menu (and maybe it's also a failure case, if the second form would actually work, since we don't seem to ever finish checking it), but it's sev:critical when you start from Account Settings on Mac, since opening autoconfig from that modal sheet opens it as a sheet, and the alert then doesn't actually show, but locks up the UI so you have to force-quit the app.
Flags: blocking-thunderbird3+
thx for filing this - I think I'm going to try to fix the pop3 protocol code so it clears the server busy flag before sending the url done notification so chaining will work.
Assignee: nobody → bienvenu
Target Milestone: --- → Thunderbird 3.0rc1
Attached patch proposed fix (obsolete) — Splinter Review
This moves the call to SetUrlState, which causes OnStopRunningUrl to be called, to after the place where we've marked the server as not busy, so if the OnStopRunningUrl callback attempts to run an other url on the server, it won't fail.  This seems like the reasonable thing to do in general. The bug could be fixed in a less general way, however, by making the autoconfig code do the second logon verification on a call to settimeout(0).

Philor, it would be great if you could verify this fixes the issue for you. I'd ask you to be the c++ reviewer, but I understand that's not your thing. And Standard8 has recently mucked about in the pop3 protocol state machine anyway :-)
Attachment #404898 - Flags: superreview?(bugzilla)
Attachment #404898 - Flags: review?(bugzilla)
Status: NEW → ASSIGNED
Whiteboard: [has patch for r/sr standard8]
Yep, fixes it perfectly for me, thx.
Whiteboard: [has patch for r/sr standard8] → [no l10n impact][has patch for r/sr standard8]
Comment on attachment 404898 [details] [diff] [review]
proposed fix

Something in this is causing test_pop3PasswordFailure.js to fail. The first error reported is:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_mailnewslocal/unit/test_pop3PasswordFailure.js | 2147500037 == 2152398850 - See following stack:
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_throw :: line 158
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 188
JS frame :: /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_mailnewslocal/unit/test_pop3PasswordFailure.js :: anonymous :: line 119

This indicates that we're now getting NS_ERROR_FAILURE rather than NS_BINDING_ABORTED.

I'm concerned that could mean we get the wrong failure codes for alerts etc.
Attachment #404898 - Flags: superreview?(bugzilla)
Attachment #404898 - Flags: review?(bugzilla)
Attachment #404898 - Flags: review-
Whiteboard: [no l10n impact][has patch for r/sr standard8] → [no l10n impact][needs new patch]
I think this means that in that error case, we're not going through the pop3 error done state. Prior to this patch, that meant the pop3 protocol state machine code wouldn't set the url state, relying on the OnStopRequest to do so.
Attached patch fix test failureSplinter Review
good catch, good tests - this should fix it.
Attachment #404898 - Attachment is obsolete: true
Attachment #406268 - Flags: superreview?(bugzilla)
Attachment #406268 - Flags: review?(bugzilla)
Whiteboard: [no l10n impact][needs new patch] → [no l10n impact][needs review standard8]
Comment on attachment 406268 [details] [diff] [review]
fix test failure

Yep, that's better.
Attachment #406268 - Flags: superreview?(bugzilla)
Attachment #406268 - Flags: superreview+
Attachment #406268 - Flags: review?(bugzilla)
Attachment #406268 - Flags: review+
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][needs review standard8] → [no l10n impact]
You need to log in before you can comment on or make changes to this bug.