[autoconfig] Race condition in addOneFinishedObserver

RESOLVED FIXED in Thunderbird 67.0

Status

enhancement
P1
major
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

(Blocks 1 bug)

Thunderbird 67.0

Thunderbird Tracking Flags

(thunderbird_esr6066+ fixed, thunderbird66 fixed, thunderbird67 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 months ago

We're not always detecting the Exchange server.

This is due to a bug in PriorityOrderAbortable.addOneFinishedObserver(). When a success callback throws, we

Expected result:
We issue 3 HTTP requests. We want to call the successCallback on the first successful HTTP request. But, successCallback might itself throw an exception, which should cause that request to be treated as a failure, and we need to move on to the next request. But if it succeeds, then any subsequent requests should be aborted.

Buggy situation:
HTTP request 2 finishes first. As it's not the first request, we don't call successCallback yet. However, the current code aborts HTTP request 3 anyway. HTTP request 1 then fails. We call successCallback on HTTP request 2, but that throws an exception. And we've already aborted HTTP request 3, which would have succeeded.

(Assignee)

Comment 1

3 months ago
Posted patch Fix, v1 (obsolete) — Splinter Review
Assignee: nobody → ben.bucksch
Status: NEW → ASSIGNED
Attachment #9043166 - Flags: review?(neil)
(Assignee)

Comment 2

3 months ago
Comment on attachment 9043166 [details] [diff] [review]
Fix, v1

Fix and bug description by Neil.
I've reviewed it.
Reverse on Bugzilla here.
(Assignee)

Comment 3

3 months ago
Posted patch Fix, v2 (obsolete) — Splinter Review
Attachment #9043166 - Attachment is obsolete: true
Attachment #9043166 - Flags: review?(neil)
Attachment #9043167 - Flags: review?(neil)
(Assignee)

Comment 4

3 months ago
Posted patch Fix, v2 (obsolete) — Splinter Review

diff with commit message

Attachment #9043167 - Attachment is obsolete: true
Attachment #9043167 - Flags: review?(neil)
(Assignee)

Comment 5

3 months ago
Posted patch Fix, v2Splinter Review
Attachment #9043169 - Attachment is obsolete: true
Attachment #9043173 - Flags: review?(neil)
(Assignee)

Updated

3 months ago
(Assignee)

Updated

3 months ago
Severity: normal → major
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Target Milestone: --- → Thunderbird 60.0
Version: Trunk → 60
Comment on attachment 9043173 [details] [diff] [review]
Fix, v2

>     +      // this is the winner
>     +      try {
>     +        successCallback(call.result, call);
>     +        haveHigherSuccess = true;
Well, it's not the winner until successCallback succeeds ;-)
Attachment #9043173 - Flags: review?(neil) → review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed
(Assignee)

Comment 7

3 months ago
Comment on attachment 9043173 [details] [diff] [review]
Fix, v2

User impact if declined: 
Exchange servers not recognized due in some cases, to race condition
Attachment #9043173 - Flags: approval-comm-esr60?
Attachment #9043173 - Flags: approval-comm-beta?

Comment 8

3 months ago
Comment on attachment 9043173 [details] [diff] [review]
Fix, v2

TB 66 beta 2. We released TB 66 beta 1 today.
Attachment #9043173 - Flags: approval-comm-beta? → approval-comm-beta+

Comment 10

3 months ago

Your HG header is nonsense :-(

# HG changeset patch
# User Ben Bucksch <ben.bucksch>
# Date 2019-02-11 16:36
commit 2fb25ac669dbd1285bd6e58093072aa99be86ac2
Author: Neil Rashbrook <neil@parkwaycc.co.uk>
Date:   Tue Feb 12 01:35:37 2019 +0100

    Bug 1527173 - [autoconfig] Race condition in addOneFinishedObserver. r=BenB

Please prepare a template header and reuse it each time, it can't be so hard.

I have the impression that Ben is the author and Neil the reviewer.

Also, the comment should reflect what you did. Also, the comments infringe English grammar :-(

Comment 11

3 months ago

And finally, you MUST supply eight lines of context, not three. Is it really so hard to get this right?

Comment 12

3 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ed4c04ff19d9
[autoconfig] Fix race condition in addOneFinishedObserver. r=Neil

Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

3 months ago
Target Milestone: Thunderbird 60.0 → Thunderbird 67.0

Updated

2 months ago
Attachment #9043173 - Flags: approval-comm-esr60? → approval-comm-esr60+
(Assignee)

Comment 15

2 months ago

Thanks, Jörg!

You need to log in before you can comment on or make changes to this bug.