1367910 - Delete active-updates.xml instead of writing to it when there isn't an active update.

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P2)

Description

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

Deleting the file is less expensive than writing to it.

Comment 1

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

Attachment: patch rev1

Since there is a backup of OS X builds and I am doing some testing on oak I pushed this to oak instead of try
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=c5ce8e336059b6fe35b108d7876f96689d163748

Comment 2

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

Attachment: patch rev2

Comment 3

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

Comment on attachment 8871562 [details] [diff] [review]
patch rev2

There is a little more that should be done here so clearing review request

Comment 4

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

Assigning as P2 since this will improve perf

Comment 5

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

Attachment: patch 1 - client code

Comment 6

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

Attachment: patch 2 - test code

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7f5906e95bb45a6c43fdf47cc4150a5a46607d

Comment 7

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8901478 [details] [diff] [review]
patch 1 - client code

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

Looks good to me, provided the below questions are answered.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2701,5 @@
>        const EMPTY_UPDATES_DOCUMENT = "<?xml version=\"1.0\"?><updates xmlns=\"http://www.mozilla.org/2005/app-update\"></updates>";
>        var doc = parser.parseFromString(EMPTY_UPDATES_DOCUMENT, "text/xml");
>  
>        for (var i = 0; i < updates.length; ++i) {
> +        // If appVersion sn't defined or the state is STATE_NONE don't include

Nit: "isn't"

@@ +2704,5 @@
>        for (var i = 0; i < updates.length; ++i) {
> +        // If appVersion sn't defined or the state is STATE_NONE don't include
> +        // the update since it is invalid.
> +        if (updates[i] && updates[i].appVersion &&
> +            updates[i].state != STATE_NONE) {

Should we also remove the file and return if all of the updates are invalid?

@@ +2732,5 @@
> +    // updated when an active update has been added to it in which case
> +    // |_updatesDirty| will be true.
> +    if (this._updatesDirty) {
> +      this._updatesDirty = false;
> +      this._writeUpdatesToXMLFile(this._updates.slice(0, 10), FILE_UPDATES_XML);

So, we're okay with STATE_NONE updates reducing the number of updates in the history file? (Since we are removing them after slicing rather than before)

Comment 8

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8901479 [details] [diff] [review]
patch 2 - test code

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

Looks good, again with the below answered.

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +1080,5 @@
>              "change after a successful update");
>  }
>  
>  /**
> + * Update Manager checks to verify that the update metadata is correct and that

Nit: "Perform Update Manager checks...". Just "Update Manager checks" sounds like Update Manager is performing the checking.

@@ +1098,5 @@
> +function checkUpdateManager(aStatusFileState, aHasActiveUpdate,
> +                            aUpdateStatusState, aUpdateErrCode, aUpdateCount) {
> +  Assert.equal(readStatusState(), aStatusFileState,
> +               "the status file state" + MSG_SHOULD_EQUAL);
> +  let msgSuffixes = [" after startup ", " after a file reload "];

Nit: msgPrefixes / msgTags? They don't seem to ever be proper suffixes.

@@ +1156,5 @@
> +  // from the xml files. This way the tests can check that the metadata for the
> +  // updates have been written to the xml files.
> +  let Cm = Components.manager.QueryInterface(Ci.nsIServiceManager);
> +  try {
> +    Cm.isServiceInstantiatedByContractID("@mozilla.org/updates/update-manager;1",

I'm not too familiar with this, but all existing usages of isServiceInstantiated(ByContractID) use the return value if it exists and false if it throws. Should this be in an if block in addition to the try?

Comment 9

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

Attachment: client code followup

I'll merge these with patch 1.

(In reply to Doug Thayer [:dthayer] from comment #7)
> Comment on attachment 8901478 [details] [diff] [review]
> patch 1 - client code
> 
> Review of attachment 8901478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, provided the below questions are answered.
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +2701,5 @@
> >        const EMPTY_UPDATES_DOCUMENT = "<?xml version=\"1.0\"?><updates xmlns=\"http://www.mozilla.org/2005/app-update\"></updates>";
> >        var doc = parser.parseFromString(EMPTY_UPDATES_DOCUMENT, "text/xml");
> >  
> >        for (var i = 0; i < updates.length; ++i) {
> > +        // If appVersion sn't defined or the state is STATE_NONE don't include
> 
> Nit: "isn't"
Fixed

> @@ +2704,5 @@
> >        for (var i = 0; i < updates.length; ++i) {
> > +        // If appVersion sn't defined or the state is STATE_NONE don't include
> > +        // the update since it is invalid.
> > +        if (updates[i] && updates[i].appVersion &&
> > +            updates[i].state != STATE_NONE) {
> 
> Should we also remove the file and return if all of the updates are invalid?
No and see below

> @@ +2732,5 @@
> > +    // updated when an active update has been added to it in which case
> > +    // |_updatesDirty| will be true.
> > +    if (this._updatesDirty) {
> > +      this._updatesDirty = false;
> > +      this._writeUpdatesToXMLFile(this._updates.slice(0, 10), FILE_UPDATES_XML);
> 
> So, we're okay with STATE_NONE updates reducing the number of updates in the
> history file? (Since we are removing them after slicing rather than before)
These exclusions were added a long time ago to try to keep only valid updates in updates.xml. An example of an invalid update is when we used to support the app.update.showInstalledUI where the active update is removed so we had to create a temporary update. The history ui already excludes these type of updates and I have been leaning towards keeping all updates in the updates.xml so it is possible to report them via telemetry.

Comment 10

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

Attachment: bug1367910-test-followup

I'll merge this with patch 2.

(In reply to Doug Thayer [:dthayer] from comment #8)
> Comment on attachment 8901479 [details] [diff] [review]
> patch 2 - test code
> 
> Review of attachment 8901479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, again with the below answered.
> 
> ::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
> @@ +1080,5 @@
> >              "change after a successful update");
> >  }
> >  
> >  /**
> > + * Update Manager checks to verify that the update metadata is correct and that
> 
> Nit: "Perform Update Manager checks...". Just "Update Manager checks" sounds
> like Update Manager is performing the checking.
Fixed

> @@ +1098,5 @@
> > +function checkUpdateManager(aStatusFileState, aHasActiveUpdate,
> > +                            aUpdateStatusState, aUpdateErrCode, aUpdateCount) {
> > +  Assert.equal(readStatusState(), aStatusFileState,
> > +               "the status file state" + MSG_SHOULD_EQUAL);
> > +  let msgSuffixes = [" after startup ", " after a file reload "];
> 
> Nit: msgPrefixes / msgTags? They don't seem to ever be proper suffixes.
Went with msgTags

> @@ +1156,5 @@
> > +  // from the xml files. This way the tests can check that the metadata for the
> > +  // updates have been written to the xml files.
> > +  let Cm = Components.manager.QueryInterface(Ci.nsIServiceManager);
> > +  try {
> > +    Cm.isServiceInstantiatedByContractID("@mozilla.org/updates/update-manager;1",
> 
> I'm not too familiar with this, but all existing usages of
> isServiceInstantiated(ByContractID) use the return value if it exists and
> false if it throws. Should this be in an if block in addition to the try?
I realized that this was only necessary for one test so I just added the call to reloadUpdateManager for that one test.

Comment 11

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

Attachment: patch 1 - client code rev2

Combined the patches and carrying forward r+

Comment 12

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

Attachment: patch 2 - test code rev2

Combined the patches and carrying forward r+

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e4127f519ecd16cba00be3f3a233d235a59c35

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00852f3e2865
client code - Bug 1367910 - Delete active-updates.xml instead of writing to it when there isn't an active update. r=dothayer
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd75aec9410
test code - Bug 1367910 - Delete active-updates.xml instead of writing to it when there isn't an active update. r=dothayer

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/00852f3e2865
https://hg.mozilla.org/mozilla-central/rev/fcd75aec9410