Last Comment Bug 689375 - Add-ons are failing to install on prod with an existing profile, no issues/errors using a fresh profile
: Add-ons are failing to install on prod with an existing profile, no issues/er...
Status: VERIFIED FIXED
[qa!]
: relnote, verified-beta
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Geoff Lankow (:darktrojan)
:
:
Mentors:
: 699816 (view as bug list)
Depends on:
Blocks: 664895
  Show dependency treegraph
 
Reported: 2011-09-26 16:45 PDT by Daniel Buchner [:dbuc]
Modified: 2013-12-27 14:36 PST (History)
14 users (show)
hskupin: in‑testsuite+
hskupin: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
unaffected


Attachments
patch (819 bytes, patch)
2011-09-29 15:15 PDT, Geoff Lankow (:darktrojan)
dtownsend: review+
Details | Diff | Splinter Review
patch with tests (3.31 KB, patch)
2011-10-05 04:42 PDT, Geoff Lankow (:darktrojan)
dtownsend: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
fixed patch (3.34 KB, patch)
2011-10-22 18:06 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
fix for windows orange (1.72 KB, patch)
2011-10-23 04:53 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch, version something (3.95 KB, patch)
2011-10-24 14:46 PDT, Geoff Lankow (:darktrojan)
dtownsend: review+
Details | Diff | Splinter Review

Description Daniel Buchner [:dbuc] 2011-09-26 16:45:57 PDT
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] 2011-09-26 17:01:06 PDT
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] 2011-09-26 17:04:28 PDT
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] 2011-09-26 17:09:27 PDT
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] 2011-09-26 17:11:00 PDT
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] 2011-09-26 17:17:52 PDT
Perhaps we should relnote it for 7 though.
Comment 6 [:Cww] 2011-09-29 14:52:16 PDT
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] 2011-09-29 14:53:38 PDT
(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) 2011-09-29 15:15:33 PDT
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.
Comment 9 Dave Townsend [:mossop] 2011-09-29 15:21:47 PDT
(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) 2011-09-29 15:36:02 PDT
(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] 2011-10-04 16:22:36 PDT
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) 2011-10-05 04:42:11 PDT
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.
Comment 13 Geoff Lankow (:darktrojan) 2011-10-05 05:25:49 PDT
(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) 2011-10-07 12:43:24 PDT
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) 2011-10-11 00:24:44 PDT
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) 2011-10-22 03:59:12 PDT
Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/0c21f57bc10c
Comment 17 Jonathan Kew (:jfkthame) 2011-10-22 05:18:37 PDT
Backed out because this caused debug xpcshell orange:
https://hg.mozilla.org/releases/mozilla-beta/rev/33cce1d6ebc3
Comment 18 Geoff Lankow (:darktrojan) 2011-10-22 18:06:14 PDT
Created attachment 568920 [details] [diff] [review]
fixed patch

Here's the extra 8 characters needed to stop this breaking on debug.
Comment 20 Geoff Lankow (:darktrojan) 2011-10-23 04:53:14 PDT
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.
Comment 21 Dão Gottwald [:dao] 2011-10-23 06:28:18 PDT
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] 2011-10-23 06:46:36 PDT
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) 2011-10-24 14:46:54 PDT
Created attachment 569193 [details] [diff] [review]
patch, version something
Comment 24 Dave Townsend [:mossop] 2011-10-26 10:04:01 PDT
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] 2011-10-28 03:16:24 PDT
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) 2011-10-31 02:52:05 PDT
(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) 2011-11-02 02:01:35 PDT
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] 2011-11-02 09:50:19 PDT
(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 christian 2011-11-02 13:43:48 PDT
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] 2011-11-02 13:46:26 PDT
(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] 2011-11-03 10:25:28 PDT
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) 2011-11-03 23:34:13 PDT
Geoff, would we have to create a manual test or does the automated test cover everything?
Comment 33 Kumar McMillan [:kumar] (needinfo all the things) 2011-11-04 13:43:29 PDT
*** Bug 699816 has been marked as a duplicate of this bug. ***
Comment 34 Geoff Lankow (:darktrojan) 2011-11-04 14:25:48 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.