Closed Bug 1572467 Opened 2 months ago Closed 2 months ago

PriorityOrderAbortable is too successful

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set

Tracking

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

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

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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 patchSplinter 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: 2 months 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.