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

VERIFIED FIXED in Firefox 8

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: dbuc, Assigned: darktrojan)

Tracking

({relnote, verified-beta})

7 Branch
mozilla8
relnote, verified-beta
Points:
---
Bug Flags:
in-testsuite +
in-litmus ?

Firefox Tracking Flags

(firefox8+ fixed, firefox9 unaffected)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → dtownsend
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?
(Reporter)

Comment 2

6 years ago
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.
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.
Blocks: 664895
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.
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
Perhaps we should relnote it for 7 though.
Keywords: relnote

Updated

6 years ago
tracking-firefox8: ? → +
tracking-firefox9: ? → +

Comment 6

6 years ago
Is there a workaround for this if you're already seeing it?  I saw this a couple times this morning.
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 563567 [details] [diff] [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.
Attachment #563567 - Flags: review?(dtownsend)
(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?
(Assignee)

Comment 10

6 years ago
(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 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.
Attachment #563567 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 564793 [details] [diff] [review]
patch with tests

I'll also add this test to mozilla-central, since the same thing is untested there.
Assignee: dtownsend → geoff
Attachment #563567 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #564793 - Flags: review?(dtownsend)
(Assignee)

Comment 13

6 years ago
(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.
Attachment #564793 - Flags: review?(dtownsend) → review+
(Assignee)

Updated

6 years ago
Attachment #564793 - Flags: approval-mozilla-beta?
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

Updated

6 years ago
Attachment #564793 - Flags: approval-mozilla-beta?
Attachment #564793 - Flags: approval-mozilla-beta+
Attachment #564793 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 15

6 years ago
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!)
tracking-firefox9: + → ---
Keywords: checkin-needed

Updated

6 years ago
Whiteboard: [land on mozilla-beta only]
Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/0c21f57bc10c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox8: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out because this caused debug xpcshell orange:
https://hg.mozilla.org/releases/mozilla-beta/rev/33cce1d6ebc3
Status: RESOLVED → REOPENED
status-firefox8: fixed → ---
Resolution: FIXED → ---
(Assignee)

Comment 18

6 years ago
Created attachment 568920 [details] [diff] [review]
fixed patch

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

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Attachment #564793 - Attachment is obsolete: true
http://hg.mozilla.org/releases/mozilla-beta/rev/b3ba14355203
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
status-firefox8: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [land on mozilla-beta only]
Target Milestone: --- → mozilla8
(Assignee)

Comment 20

6 years ago
Created attachment 568940 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
I won't push unreviewed bustage fixes to mozilla-beta for code that I don't know, sorry. Backed out.
Status: RESOLVED → REOPENED
status-firefox8: fixed → ---
Keywords: checkin-needed
Resolution: FIXED → ---
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.
(Assignee)

Comment 23

6 years ago
Created attachment 569193 [details] [diff] [review]
patch, version something
Attachment #568920 - Attachment is obsolete: true
Attachment #568940 - Attachment is obsolete: true
Attachment #569193 - Flags: review?(dtownsend)
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
Attachment #569193 - Flags: review?(dtownsend) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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 :-)
(Assignee)

Comment 26

6 years ago
(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.
Whiteboard: [land on mozilla-beta only]
(Assignee)

Comment 27

6 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [land on mozilla-beta only]
(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

6 years ago
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".
(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

6 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [qa!]
Geoff, would we have to create a manual test or does the automated test cover everything?
Flags: in-testsuite+
Flags: in-litmus?
Duplicate of this bug: 699816
(Assignee)

Comment 34

6 years ago
(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.
(Assignee)

Updated

6 years ago
status-firefox9: --- → unaffected
status-firefox8: --- → fixed
You need to log in before you can comment on or make changes to this bug.