Closed Bug 1572467 Opened 4 years ago Closed 4 years ago

PriorityOrderAbortable is too successful

Categories

(Thunderbird :: Account Manager, defect)

Product:
Thunderbird
Component:
Account Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6069+ fixed, thunderbird_esr6869+ fixed, thunderbird69 fixed, thunderbird70 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0
Tracking Flags:
Tracking Status
thunderbird_esr60 69+ fixed
thunderbird_esr68 69+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file, 1 obsolete file)

Proposed patch
1.88 KB, patch
BenB
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr60+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

PriorityOrderAbortable wants to call successCallback on all successful calls in priority order until it finds one for which successCallback itself succeeds.

However, having found a successful call with a successful callback, it proceeds to call this callback again every time any of its other calls completes for any reason.

Attached patch Proposed patch — — Splinter Review

The variable that keeps track of which call's callback was successful is local to the observer, so we forget that we called it. Instead, once we know that a success callback succeeded, we shouldn't call the success callback again.

We don't have to store the successful call itself but I did just in case it could be of interest.

Assignee: nobody → neil
Attachment #9084039 - Flags: review?(ben.bucksch)
Comment on attachment 9084039 [details] [diff] [review]
Proposed patch

Ben gave me r+ over the phone as an interim measure but we have an idea for an alternative version.
Attachment #9084039 - Flags: review?(ben.bucksch) → review+
Keywords: checkin-needed
Attached patch Alternative approach (obsolete) — — Splinter Review
Attachment #9085090 - Flags: review?(ben.bucksch)

Note that while testing these patches I noticed that they exacerbate a Trunk-only bug whereby the Exchange autodiscovery can continue after another method has already found a configuration. Without this patch, we refresh the UI after that discovery finishes, but these patches optimise away that refresh...

Comment on attachment 9085090 [details] [diff] [review]
Alternative approach

Ben decided not to go with this version as the other version was clearer.
Attachment #9085090 - Attachment is obsolete: true
Attachment #9085090 - Flags: review?(ben.bucksch)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d924fa692a42
Don't call successCallback again once it succeeds. r=BenB

Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Comment on attachment 9084039 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): Bug in new code added by 1500105
User impact if declined: Not visible to user, causes bug 1571772 to make unnecessary requests
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Attachment #9084039 - Flags: approval-comm-beta?
Attachment #9084039 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9084039 - Flags: approval-comm-esr68?
Attachment #9084039 - Flags: approval-comm-esr60?
Attachment #9084039 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9084039 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.