1596000 - [autoconfig] Avoid double OPTIONS request

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[Ben Bucksch (:BenB)]

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.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Avoid double OPTIONS when not necessary, v1

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.

Comment 2

[Ben Bucksch (:BenB)]

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.

Comment 3

[Ben Bucksch (:BenB)]

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.

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Avoid double OPTIONS when not necessary, v2

Linted

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Avoid double OPTIONS when not necessary, v3

Comment 6

[Jorg K (CEST = GMT+2)]

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?

Comment 7

[Ben Bucksch (:BenB)]

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.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0402e4370a3c
Avoid double pre-flight OPTIONS request when not necessary. r=Neil

Comment 10

[Ben Bucksch (:BenB)]

Thanks :)

Comment 11

[Jorg K (CEST = GMT+2)]

TB 68.3 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/3eefe4256a08b60ddb8fca92b2d1e652d9c431e4

Comment 12

[Jorg K (CEST = GMT+2)]

TB 71 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/db6162333c542d956ba8f43f9d94a6abfdf10686