Closed Bug 1596000 Opened 11 months ago Closed 11 months ago

[autoconfig] Avoid double OPTIONS request

Categories

(Thunderbird :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr6871+ fixed, thunderbird71 fixed, thunderbird72 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 71+ fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: BenB, Assigned: BenB)

References

(Blocks 1 open bug, Regression)

Details

Attachments

(1 file, 2 obsolete files)

When fixing bug 1572418, we made pre-flight requests to both the ISPDB and addons.json, pre-warming caches for both. In theory, they could be on different servers, so Neil made these 2 requests, simply for correctness. In practice, they are on the same server. So, we could reduce this to a single OPTIONS requests, at least when both are on the same server. We just need to pre-warm DNS and OCSP, so we could avoid one of the calls, if they are on the same server.

Assignee: nobody → ben.bucksch
Status: NEW → ASSIGNED
Summary: [autoconfig] Avoid OPTIONS request to addons.json → [autoconfig] Avoid double OPTIONS request

This avoids the second OPTIONS request to addons.json, if it's on the same host as the ISPDB. In this case, the call is unnecessary, so skip it.

Attachment #9108323 - Flags: review?(neil)

Patch is dry-coded and untested, because TB trunk fails to compile for me. Putting it up for demonstration of good will, and for code review.

Regressed by: 1572418

TB trunk builds for me now. Tested and fix works as expected. If both URLs are on the same host, only the ISPDB is pre-warmed. If they are on different hosts (I tested with live.thunderbird.net and autoconfig.thunderbird.net in the prefs), I get both pre-warm calls. Note that bug 1572418 found that addons.json is also affected, so this check is necessary.

We now avoid the pre-warm call to addons.json whenever possible.

Linted

Attachment #9108323 - Attachment is obsolete: true
Attachment #9108323 - Flags: review?(neil)
Attachment #9108428 - Flags: review?(neil)
Attachment #9108428 - Attachment is patch: true
Attachment #9108428 - Attachment is obsolete: true
Attachment #9108428 - Flags: review?(neil)
Attachment #9108430 - Flags: review?(neil)
Comment on attachment 9108428 [details] [diff] [review]
Avoid double OPTIONS when not necessary, v2

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

::: mail/components/accountcreation/content/emailWizard.js
@@ +288,4 @@
>        method: "OPTIONS",
>      });
> +    let addonsURL = Services.prefs.getCharPref("mailnews.auto_config.addons_url");
> +    if (new URL(autoconfigURL).origin != new URL(addonsURL).origin) {

Do you really have to create URLs and the compare the origins? A string compare wouldn't do it?
Attachment #9108428 - Attachment is obsolete: false
Attachment #9108430 - Flags: review?(neil) → review+

A string compare wouldn't do it?

The URLs are not the same, the paths are different. I could of course parse out the origin of the URLs manually, but that's what URL is for. It avoids hacky URL parsing, makes readable code, helps a lot in such cases and I'm quite glad it's part of the JS environment now.

Attachment #9108428 - Attachment is obsolete: true
Comment on attachment 9108430 [details] [diff] [review]
Avoid double OPTIONS when not necessary, v3

OK, we might as well push this improvement to the branches.
Attachment #9108430 - Flags: approval-comm-esr68+
Attachment #9108430 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 72.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0402e4370a3c
Avoid double pre-flight OPTIONS request when not necessary. r=Neil

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED

Thanks :)

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