576939 - Backport app update fixes to 1.9.2

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

There were several fixes to app update that were at first planned for 1.9.2.4 (see rollup patch in bug 530872) but to lessen the risk for the 1.9.2.4 it was decided they shouldn't land. This bug is for getting these fixes landed along with tests.

Comment 1

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

Attachment: patch - app changes

Comment 2

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

Attachment: patch - tests

Comment 3

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

Comment on attachment 456101 [details] [diff] [review]
patch - app changes

note: a large portion of the changes are necessary to make running mochitests sane and to make the code consistent with trunk so it is easier to compare a diff between 1.9.2 and trunk.

Comment 4

[Dave Townsend [:mossop]]

Rob, would it be a problem for me to hold off reviewing this till the middle of next week? I have a bunch of stuff that could do with getting done before the b2 freeze on Monday.

Comment 5

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

Sure... b2 should take priority.

Comment 6

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

Comment on attachment 456102 [details] [diff] [review]
patch - tests

I've changed the following to false locally
const DEBUG_DUMP = true;

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 456101 [details] [diff] [review]
patch - app changes

As far as I can see this looks right, but there is a lot of code here. I think we should ensure that we focus heavily on manually testing the update system on branch after this lands before it makes a release.

Comment 8

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

Comment on attachment 456102 [details] [diff] [review]
patch - tests

Drivers, this is a rollup patch for bug fixes that we were originally going to land for 3.6.4 but it was decided 3.6.4 was already risky enough with the OOPP work. The primary bugs this fixes are bug 485493, bug 536547, bug 540046, bug 551283, and bug 563810. With this fixed we will never get the license not found bugs that crop up periodically as well. The second patch contains a bunch of new tests for verifying correct UI behavior which is a very good thing but it required significant cleanup of the ui code which is why the patch is large. There have been requests to backport bug 538331, bug 480178, and bug 530872 to 1.9.2 which I would not be comfortable doing without this landing on 1.9.2 due to the bugs this fixes, the tests, and that this will make the 1.9.2 code similar to the trunk code that has been baking for several months.

Besides the tests I have manually tested the different update scenarios and to mitigate it further QA and myself should run through those tests again.

Comment 9

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

(In reply to comment #8)
> Besides the tests I have manually tested the different update scenarios and to
> mitigate it further QA and myself should run through those tests again.

We will also have to backport all the changes for Mozmill to get the tests working for 1.9.2 builds. There is still bug 514040 which blocks us from running fallback updates against complete updates. That would probably introduce manual testing for the beginning of those update paths.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Besides the tests I have manually tested the different update scenarios and to
> > mitigate it further QA and myself should run through those tests again.
> 
> We will also have to backport all the changes for Mozmill to get the tests
> working for 1.9.2 builds.
Since 1.9.2 hasn't changed I don't see why you *actually* need this on 1.9.2 to get mozmill tests working unless mozmill has been broken since Firefox 3.6 released. Can you provide details / specifics?

> There is still bug 514040 which blocks us from
> running fallback updates against complete updates. That would probably
> introduce manual testing for the beginning of those update paths.
How does bug 514040 prevent you from doing fallback testing? With this patch and the patch in bug 514040 you should be able to get mozmill working with minor changes to the current trunk verssion of mozmill.

Comment 11

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

(In reply to comment #10)
> Since 1.9.2 hasn't changed I don't see why you *actually* need this on 1.9.2 to
> get mozmill tests working unless mozmill has been broken since Firefox 3.6
> released. Can you provide details / specifics?

Does it mean we do not land the Billboard changes on 1.9.2? If not, you are right. Otherwise our tests have to be updated to use the new pages (update available, error) which have been introduced with it on trunk.

> How does bug 514040 prevent you from doing fallback testing? With this patch
> and the patch in bug 514040 you should be able to get mozmill working with
> minor changes to the current trunk verssion of mozmill.

Well, once the backported patches get checked in (and the above mentioned condition is met) we will have the same problems as on trunk. The updater is using a partial update instead of the complete one for update paths like 3.6.7 -> 3.6.9. The transition period will definitely involve some manual testing, because updating our SoftwareUpdateAPI will make it incompatbile with older versions (< 3.6.9)

Comment 12

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

(In reply to comment #11)
> Does it mean we do not land the Billboard changes on 1.9.2? If not, you are
> right. Otherwise our tests have to be updated to use the new pages (update
> available, error) which have been introduced with it on trunk.

While checking the patch I can see those changes, which means we have to update our Mozmill tests to handle it gracefully.

Comment 13

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

(In reply to comment #12)
> (In reply to comment #11)
> > Does it mean we do not land the Billboard changes on 1.9.2? If not, you are
> > right. Otherwise our tests have to be updated to use the new pages (update
> > available, error) which have been introduced with it on trunk.
> 
> While checking the patch I can see those changes, which means we have to update
> our Mozmill tests to handle it gracefully.
Which changes specifically?

If mozmill is going to use the same version to test different branches I highly recommend that you rethink that and either do version detection with different code paths or use different versions for each branch. The problems with not doing so with other parts of our processes have been very problematic in the past.

Comment 14

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

Henrik, please file a new bug against app update to have me review / fixup the app update mozmill tests.

Comment 15

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

(In reply to comment #13)
> Which changes specifically?

For the billboard, especially the handling of the new wizard pages.

> If mozmill is going to use the same version to test different branches I highly
> recommend that you rethink that and either do version detection with different
> code paths or use different versions for each branch. The problems with not
> doing so with other parts of our processes have been very problematic in the
> past.

We have different versions of the shared modules for each branch. But ui updates also requires updates to our shared modules. All the work I will track on bug 567258. I will cover the necessary work, so you don't have to "waste" your time on it.

Comment 16

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

After taking a look at the tests I am pretty sure it will be easy to make them work on 1.9.2 and trunk with or without this patch

Comment 17

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

(In reply to comment #16)
> After taking a look at the tests I am pretty sure it will be easy to make them
> work on 1.9.2 and trunk with or without this patch
Drivers, I am quite certain this statement is correct and I have submitted a patch to fix a pre-existing bug in mozmill in bug 567258. The relevant comment in regards to whether this bug should be a+ or a- is comment #8.

Comment 18

[LegNeato]

Comment on attachment 456102 [details] [diff] [review]
patch - tests

Though it scares me to do it, a=LegNeato for 1.9.2.9. I think the extra play in the schedule will be beneficial for testing this bug and making sure the tools are correct. Please land on mozilla-1.9.2 default as soon as you can.

Comment 19

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

Comment on attachment 456101 [details] [diff] [review]
patch - app changes

LegNeato, sorry about that... need approval for the client code patch as well.

Comment 20

[LegNeato]

Yeah, whoops, I should have seen that.

Comment 21

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

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/75c145d5e0b5
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/222bcfc9fc8c

I've verified that the patch in bug 567258 fixes mozmill using mozmill tip. I'll backport that to mozmill mozilla1.9.2 as well.

I've manually verified all of the update scenarios with the patch and also checked that there were no warnings or errors for each of the manual checks.

I've ran all of the app update tests using both an optimized and a debug builds multiple time successfully. I also verified there were no warnings or errors that aren't expected when running the tests.

Comment 26

[Masahiro YAMADA]

/toolkit/mozapps/update/src/nsUpdateService.js.in
>
>  // Set the initial value with the current time when it doesn't already have a
>  // value or the value is already set to 0 (bug 316328).
>  if (!this.installDate && this.installDate != 0)
>    this.installDate = (new Date()).getTime();

Comment suggests fooowing code:

>  if ((!this.installDate) || (this.installDate == 0))
>    this.installDate = (new Date()).getTime();

Which is correct ?

Comment 27

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

The code is since !this.installDate will evaluate to true but we don't want to set it if it is already 0 since bug caused previous updates to set installDate to 0 and this shouldn't reset the value to the date / time when the updates.xml is loaded