1380278 - UpdateUtils.jsm does main thread I/O to read the update.locale file

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P1)

Description

[Florian Quèze [:florian]]

The code at http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/toolkit/modules/UpdateUtils.jsm#99 does main thread I/O.

We should stop doing main thread I/O for this, and likely stop using NetUtil there.

UpdateUtils.Locale is apparently only used by UpdateUtils.formatUpdateURL, which only has 3 callers (+ 2 from tests) : http://searchfox.org/mozilla-central/search?q=formatUpdateURL

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'll take it.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review commit: https://reviewboard.mozilla.org/r/156588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156588/

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/1-2/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Florian - does it look like what you'd expect to see?

I switched to async I/O using Fetch API instead of NetUtils.

I had to update all the callers to be async as well, which fortunately wasn't too hard.

I didn't touch the tests yet, because one of them is a marionette test which may be tricky to handle with async, and the other is just a bit of manual labor to switch, so decided to get your feedback first to see if I'm moving in the right direction.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/2-3/

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review161774

::: toolkit/mozapps/update/nsUpdateService.js:2034
(Diff revision 3)
>        }
>        return;
>      }
>  
>      let validUpdateURL = true;
> -    try {
> +    this.getUpdateURL(false).catch(e => {

Should this be this.backgroundChecker.getUpdateURL?

::: toolkit/mozapps/update/nsUpdateService.js:2963
(Diff revision 3)
> -      }
> +        }
>  
> -      // Tell the callback about the updates
> +        // Tell the callback about the updates
> -      this._callback.onCheckComplete(event.target, updates, updates.length);
> +        this._callback.onCheckComplete(event.target, updates, updates.length);
> +      });
>      } catch (e) {

Looks like this catch wants to be chained to the then()

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> Comment on attachment 8885807 [details]
> Bug 1380278 - UpdateUtils.jsm does main thread I/O to read the update.locale
> file.
> 
> Florian - does it look like what you'd expect to see?

Yes, thanks :-)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/3-4/

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review161794

::: toolkit/mozapps/update/nsUpdateService.js:2962
(Diff revisions 3 - 4)
> -        }
> +      }
>  
> -        // Tell the callback about the updates
> +      // Tell the callback about the updates
> -        this._callback.onCheckComplete(event.target, updates, updates.length);
> +      this._callback.onCheckComplete(event.target, updates, updates.length);
> -      });
> -    } catch (e) {
> +      this._callback = null;
> +      this._request = null;

Instead of duplicating these 2 lines, another option is to chain another .then after the .catch

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/4-5/

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/5-6/

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks! I updated the patch and switched the tests. Hopefully the try run will come clean :)

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/6-7/

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review162000

Looks good overall :-)

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:380
(Diff revision 7)
>          :param force: Boolean flag to force an update check
>  
>          :returns: The URL of the update snippet
>          """
>          # Format the URL by replacing placeholders
> -        url = self.marionette.execute_script("""
> +        url = self.marionette.execute_async_script("""

I'm not familiar enough with marionette to feel confident reviewing this change.

The documentation at http://searchfox.org/mozilla-central/source/testing/marionette/client/docs/basics.rst says "The async method works the same way, except it won't return until a special marionetteScriptFinished() function is called". I'm not sure if we need to do it.

::: toolkit/modules/UpdateUtils.jsm:104
(Diff revision 7)
> - * Gets the locale from the update.locale file for replacing %LOCALE% in the
> +   * Gets the locale from the update.locale file for replacing %LOCALE% in the
> - * update url. The update.locale file can be located in the application
> +   * update url. The update.locale file can be located in the application
> - * directory or the GRE directory with preference given to it being located in
> +   * directory or the GRE directory with preference given to it being located in
> - * the application directory.
> +   * the application directory.
> - */
> +   */
> -XPCOMUtils.defineLazyGetter(UpdateUtils, "Locale", function() {
> +  async getLocale() {

The previous code using a lazy getter was caching the result so that we would only need to do I/O once. Should we preserve this behavior even if I/O is non blocking now? How likely are we to call formatUpdateURL several times?

::: toolkit/mozapps/update/nsUpdateService.js:2850
(Diff revision 7)
>      LOG("Checker: checkForUpdates, force: " + force);
>      if (!listener)
>        throw Cr.NS_ERROR_NULL_POINTER;
>  
> -    var url = this.getUpdateURL(force);
> +    this.getUpdateURL(force).then(url => {
> -    if (!url || (!this.enabled && !force))
> +      if (!url || (!this.enabled && !force))

Should we do the (!this.enabled && !force) check and early return before getting the URL?

::: toolkit/mozapps/update/nsUpdateService.js:2916
(Diff revision 7)
>          update = new Update(updateElement);
>        } catch (e) {
>          LOG("Checker:_updates get - invalid <update/>, ignoring...");
>          continue;
>        }
> -      update.serviceURL = this.getUpdateURL(this._forced);
> +      update.serviceURL = await this.getUpdateURL(this._forced);

I don't think we should re-generate the update url while parsing the downloaded XML. It should be possible to either get this url from the this._request object, or to cache it at the time we start the XHR.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/7-8/

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review162248

Looks good to me, thanks! I think the marionette code is now correct, but I would still prefer if someone else could r+ it.

::: toolkit/modules/UpdateUtils.jsm:107
(Diff revisions 7 - 8)
>     * update url. The update.locale file can be located in the application
>     * directory or the GRE directory with preference given to it being located in
>     * the application directory.
>     */
>    async getLocale() {
> +    if (this._locale === undefined) {

I think you meant !==

::: toolkit/mozapps/update/nsUpdateService.js:2849
(Diff revisions 7 - 8)
>    checkForUpdates: function UC_checkForUpdates(listener, force) {
>      LOG("Checker: checkForUpdates, force: " + force);
>      if (!listener)
>        throw Cr.NS_ERROR_NULL_POINTER;
>  
> +    if (!this.enabled && !force) {

nit: I would match the existing style of the surrounding code (eg. 3 lines above) and not add the {} for a single line if.

::: toolkit/mozapps/update/nsUpdateService.js:2854
(Diff revisions 7 - 8)
> +    if (!this.enabled && !force) {
> +      return;
> +    }
> +
>      this.getUpdateURL(force).then(url => {
> -      if (!url || (!this.enabled && !force))
> +      if (!url) {

same here

::: toolkit/mozapps/update/nsUpdateService.js
(Diff revision 8)
>          update.errorCode = HTTP_ERROR_OFFSET + status;
>        }
>  
>        this._callback.onError(request, update);
>      }
> -

I would revert this whitespace only change in this code that we are otherwise not touching.

Comment 17

[Robert Strong (they/them - no direct email)]

Please add braces to any changed code in nsUpdateService.js.

Comment 18

[Florian Quèze [:florian]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #17)
> Please add braces to any changed code in nsUpdateService.js.

Then let's also add them 3 lines above.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/8-9/

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Henrik - you seem to touch that file often. Can you look at the change in the marionette script?

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/9-10/

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/10-11/

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review162502

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:384
(Diff revision 11)
>          # Format the URL by replacing placeholders
> -        url = self.marionette.execute_script("""
> -          Components.utils.import("resource://gre/modules/UpdateUtils.jsm")
> -          return UpdateUtils.formatUpdateURL(arguments[0]);
> +        url = self.marionette.execute_async_script("""
> +          Components.utils.import("resource://gre/modules/UpdateUtils.jsm");
> +          UpdateUtils.formatUpdateURL(arguments[0]).then(url => {
> +            marionetteScriptFinished(url);
> +          });

This code runs for update tests of Firefox which means it would also have to run for previous versions than 56. I doubt that this will work, so we should most likely add a version check, and run different code dependent on the current version.

For Nightly I would expect broken update tests for the next 4 days after this patch landed, or we also include the exact day but that seems to be too much here.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/11-12/

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review162502

> This code runs for update tests of Firefox which means it would also have to run for previous versions than 56. I doubt that this will work, so we should most likely add a version check, and run different code dependent on the current version.
> 
> For Nightly I would expect broken update tests for the next 4 days after this patch landed, or we also include the exact day but that seems to be too much here.

Thanks Henrik! I updated the marionette patch to handle both.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

https://reviewboard.mozilla.org/r/156588/#review162578

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:381
(Diff revisions 11 - 12)
>  
>          :returns: The URL of the update snippet
>          """
>          # Format the URL by replacing placeholders
> +        # In 56 we switched the method to be async.
> +        # For now, support both approaches.

Could you please move those comments into the JS code?

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:384
(Diff revisions 11 - 12)
>          # Format the URL by replacing placeholders
> +        # In 56 we switched the method to be async.
> +        # For now, support both approaches.
>          url = self.marionette.execute_async_script("""
>            Components.utils.import("resource://gre/modules/UpdateUtils.jsm");
> -          UpdateUtils.formatUpdateURL(arguments[0]).then(url => {
> +          let res = UpdateUtils.formatUpdateURL(arguments[0]);

An even better approach, which will always work! Thanks a lot.

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8885807 [details]
Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156588/diff/12-13/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fca3daebf2b
UpdateUtils.getLocale to Fetch API for async I/O. r=florian,whimboo

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fca3daebf2b