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

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Application Update
P2
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Deleting the file is less expensive than writing to it.
Created attachment 8871492 [details] [diff] [review]
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
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Created attachment 8871562 [details] [diff] [review]
patch rev2
Attachment #8871492 - Attachment is obsolete: true
Attachment #8871562 - Flags: review?(mhowell)
Comment on attachment 8871562 [details] [diff] [review]
patch rev2

There is a little more that should be done here so clearing review request
Attachment #8871562 - Attachment is obsolete: true
Attachment #8871562 - Flags: review?(mhowell)
Assigning as P2 since this will improve perf
Priority: -- → P2
Blocks: 1355956, 1361102
Created attachment 8901478 [details] [diff] [review]
patch 1 - client code
Attachment #8901478 - Flags: review?(dothayer)
Created attachment 8901479 [details] [diff] [review]
patch 2 - test code

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7f5906e95bb45a6c43fdf47cc4150a5a46607d
Attachment #8901479 - Flags: review?(dothayer)
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)
Attachment #8901478 - Flags: review?(dothayer) → review+
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?
Attachment #8901479 - Flags: review?(dothayer) → review+
Created attachment 8902018 [details] [diff] [review]
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.
Attachment #8902018 - Flags: review?(dothayer)
Created attachment 8902020 [details] [diff] [review]
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.
Attachment #8902020 - Flags: review?(dothayer)

Updated

4 months ago
Attachment #8902018 - Flags: review?(dothayer) → review+

Updated

4 months ago
Attachment #8902020 - Flags: review?(dothayer) → review+
Created attachment 8902057 [details] [diff] [review]
patch 1 - client code rev2

Combined the patches and carrying forward r+
Attachment #8901478 - Attachment is obsolete: true
Attachment #8902018 - Attachment is obsolete: true
Attachment #8902057 - Flags: review+
Created attachment 8902058 [details] [diff] [review]
patch 2 - test code rev2

Combined the patches and carrying forward r+

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e4127f519ecd16cba00be3f3a233d235a59c35
Attachment #8901479 - Attachment is obsolete: true
Attachment #8902020 - Attachment is obsolete: true
Attachment #8902058 - Flags: review+

Comment 13

3 months ago
Pushed by rstrong@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/00852f3e2865
https://hg.mozilla.org/mozilla-central/rev/fcd75aec9410
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.