689375 - Add-ons are failing to install on prod with an existing profile, no issues/errors using a fresh profile

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Buchner [:dbuc]]

All my existing profiles for Firefox 8 and lower will not install add-ons and throw a couple different errors, but all from a single file: resource://gre/modules/AddonRepository.jsm

I currently have folks able to replicate this *100% of the time* in Windows 7, and some on OSX.

Steps to reproduce:

1. Start up any version of Firefox from 7 to Aurora
2. Install any add-on from AMO
3. In the error console you will see exceptions raised on either line 1375 or 1556 of AddonRepository.jsm (those are the two types I have seen and others are reporting)

Please advise.

Comment 1

[Dave Townsend [:mossop]]

Are you using a profile that was previously used with Firefox 9? We upgraded a database in Firefox 9 and I bet it doesn't handle downgrades well at all.

What are the exact errors you are seeing?

Comment 2

[Daniel Buchner [:dbuc]]

Yes, this profile has been used on all builds, from 6 - 9.

Some errors were in relation to aSql calls on line 1375, and others were related to add-on insertion into the db through the transaction interface method found in the line 1556 code block.

Comment 3

[Dave Townsend [:mossop]]

Ok, this is most likely fallout from bug 664895. We can't make this work in a way that downgrading directly from Firefox 9 to Firefox 6 would work. Since Firefox 7 is basically ready for release we probably can't for that either. We could port part of the work in bug 664895 handling schema changes to Firefox 8 which would allow a user to downgrade from Firefox 9 to 8 (and from there to 7 and lower) without error. We could also push bug 664895 to a later release and potentially still do the backport work to 8 and increase the working downgrade window.

Comment 4

[Dave Townsend [:mossop]]

We don't need to block Firefox 7's release on this, we should probably decide on a timeline for the solution and do any appropriate work for 8 though.

Comment 5

[Dave Townsend [:mossop]]

Perhaps we should relnote it for 7 though.

Comment 6

[[:Cww]]

Is there a workaround for this if you're already seeing it?  I saw this a couple times this morning.

Comment 7

[Dave Townsend [:mossop]]

(In reply to [:Cww] from comment #6)
> Is there a workaround for this if you're already seeing it?  I saw this a
> couple times this morning.

Deleting addons.sqlite from the profile should suffice.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch

I haven't got a mozilla-beta tree, so this patch is faked somewhat, but I think this is all it would take to fix the bug.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Geoff Lankow (:darktrojan) from comment #8)
> Created attachment 563567 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> I haven't got a mozilla-beta tree, so this patch is faked somewhat, but I
> think this is all it would take to fix the bug.

Won't that just leave the database unusable after that point?

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Dave Townsend (:Mossop) from comment #9)
> (In reply to Geoff Lankow (:darktrojan) from comment #8)
> > Created attachment 563567 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > I haven't got a mozilla-beta tree, so this patch is faked somewhat, but I
> > think this is all it would take to fix the bug.
> 
> Won't that just leave the database unusable after that point?

The rest of that catch block is:

>      LOG("Deleting database, and attempting openConnection again");
>      dbfile.remove(false);
>      return this.openConnection(true);

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 563567 [details] [diff] [review]
patch

That's what I get for reviewing while on vacation. This does look ok, can we get a simple test for this? Should just be a case of manually creating a DB with a high schema then starting up the add-ons manager and make sure the cache still works and add-ons can still be installed.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: patch with tests

I'll also add this test to mozilla-central, since the same thing is untested there.

Comment 13

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #12)
> Created attachment 564793 [details] [diff] [review] [diff] [details] [review]
> patch with tests
> 
> I'll also add this test to mozilla-central, since the same thing is untested
> there.

And when I discover it's broken too, I'll file bug 692078.

Comment 14

[Matt Brubeck (:mbrubeck)]

Note for anyone following the link from this changeset:
https://hg.mozilla.org/mozilla-central/rev/ef21d6f2187d

It was backed out and checked in with the correct bug number (Bug 692078):
https://hg.mozilla.org/mozilla-central/rev/97f2cd5ea1ee

Comment 15

[Geoff Lankow (:darktrojan)]

This patch can now land on mozilla-beta only. Bug 692078 is dealing with version 9 and up. (Thanks for the aurora approval anyway, LegNeato. It's the thought that counts!)

Comment 16

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/0c21f57bc10c

Comment 17

[Jonathan Kew [:jfkthame]]

Backed out because this caused debug xpcshell orange:
https://hg.mozilla.org/releases/mozilla-beta/rev/33cce1d6ebc3

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: fixed patch

Here's the extra 8 characters needed to stop this breaking on debug.

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-beta/rev/b3ba14355203

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: fix for windows orange

Still causing orange on Windows, I forgot to close the connection before trying to remove the file. Also, there's a typo in older code hiding the real problem.

Comment 21

[Dão Gottwald [:dao]]

I won't push unreviewed bustage fixes to mozilla-beta for code that I don't know, sorry. Backed out.

Comment 22

[Bill Gianopoulos [:WG9s]]

I think the issue here is that if it were a test only fix, it might have been OK, but as it is a change to the code, it definitely needs review first.

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: patch, version something

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 569193 [details] [diff] [review]
patch, version something

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1411,5 @@
>      let sql = this.statements[aKey];
>      try {
>        return this.statementCache[aKey] = this.connection.createStatement(sql);
>      } catch (e) {
> +      ERROR("Error creating statement " + aKey + " (" + sql + ")");

This already got fixed in bug 683129

Comment 25

[Ed Morley [:emorley]]

Where should this be landed? mozilla-beta again?
Has it been sent to try?
Will it still apply cleanly, given comment 24? (unfortunately I don't have a beta tree to hand, to find out)

Please be as explicit as possible when requesting checkin-needed, especially for patches that have been backed out previously. Also, attaching revised patches is often a good chance to add the |; r=foo| to the commit message, to save the pusher from doing it.

Thanks :-)

Comment 26

[Geoff Lankow (:darktrojan)]

(In reply to Ed Morley [:edmorley] from comment #25)
> Where should this be landed? mozilla-beta again?
Yes.

> Has it been sent to try?
Yes. https://tbpl.mozilla.org/?tree=Try&rev=eff30e589945

> Will it still apply cleanly, given comment 24? (unfortunately I don't have a
> beta tree to hand, to find out)
Yes, that never landed on 8.

Comment 27

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/releases/mozilla-beta/rev/296e61cb2819

I've pushed this despite being past the deadline, in case another beta is made. I haven't set status-firefox8 to fixed, in case another beta isn't made.

Comment 28

[Dave Townsend [:mossop]]

(In reply to Geoff Lankow (:darktrojan) from comment #27)
> https://hg.mozilla.org/releases/mozilla-beta/rev/296e61cb2819
> 
> I've pushed this despite being past the deadline, in case another beta is
> made. I haven't set status-firefox8 to fixed, in case another beta isn't
> made.

Uhh this patch wasn't approved for mozilla-beta. Christian are you happy with this landing at this point?

Comment 29

[LegNeato]

Yeah, I think we can afford to take this.  I don't see where it landed on mozilla-central, please make sure it is there (and aurora too).

If others disagree I'll just back it out of mozilla-beta:-). Thanks for pointing out a hole in the process...I need to close the tree when we think we are "done".

Comment 30

[Dave Townsend [:mossop]]

(In reply to Christian Legnitto [:LegNeato] from comment #29)
> Yeah, I think we can afford to take this.  I don't see where it landed on
> mozilla-central, please make sure it is there (and aurora too).

It only landed on beta because that is the only place that needs the fix.

Comment 31

[Vlad [QA]]

I've tested this issue on:
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0 beta 6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0 beta 6
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0 beta 6
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0 beta 6

and the errors from the description are not present.

Considering comment30, setting resolution to Verified Fixed on Beta.

Comment 32

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

Geoff, would we have to create a manual test or does the automated test cover everything?

Comment 34

[Geoff Lankow (:darktrojan)]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Geoff, would we have to create a manual test or does the automated test
> cover everything?

I'm pretty sure the automated test covers it.