Closed Bug 1380278 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Application Update, enhancement, P1)

Product:
Toolkit
Component:
Application Update
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Iteration:
56.3 - Jul 24
Project Flags:
Performance Impact ?
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: florian, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 file)

Bug 1380278 - UpdateUtils.getLocale to Fetch API for async I/O.
59 bytes, text/x-review-board-request
florian
: review+
whimboo
: review+
Details 
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
Flags: qe-verify-
Priority: -- → P3
I'll take it.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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.
Attachment #8885807 - Flags: feedback?(florian)
Priority: P3 → P1
Attachment #8885807 - Flags: feedback?(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 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 :-)
Attachment #8885807 - Flags: feedback?(florian) → feedback+
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
Thanks! I updated the patch and switched the tests. Hopefully the try run will come clean :)
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.
Attachment #8885807 - Flags: review?(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.
Attachment #8885807 - Flags: review?(florian) → review+
Please add braces to any changed code in nsUpdateService.js.
(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 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?
Attachment #8885807 - Flags: review?(hskupin)
Attachment #8885807 - Flags: review?(hskupin)
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.
Attachment #8885807 - Flags: review?(hskupin) → review-
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 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.
Attachment #8885807 - Flags: review?(hskupin) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fca3daebf2b
UpdateUtils.getLocale to Fetch API for async I/O. r=florian,whimboo
https://hg.mozilla.org/mozilla-central/rev/5fca3daebf2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Iteration: --- → 56.3 - Jul 24
Performance Impact: --- → ?
Whiteboard: [reserve-photon-performance][qf] → [reserve-photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: