Closed
Bug 689375
Opened 13 years ago
Closed 13 years ago
Add-ons are failing to install on prod with an existing profile, no issues/errors using a fresh profile
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
VERIFIED
FIXED
mozilla8
Tracking | Status | |
---|---|---|
firefox8 | + | fixed |
firefox9 | --- | unaffected |
People
(Reporter: dbuchner, Assigned: darktrojan)
References
Details
(Keywords: relnote, verified-beta, Whiteboard: [qa!])
Attachments
(1 file, 4 obsolete files)
3.95 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Assignee: nobody → dtownsend
Comment 1•13 years ago
|
||
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•13 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.
Comment 3•13 years ago
|
||
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
Comment 4•13 years ago
|
||
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:
--- → ?
Updated•13 years ago
|
Is there a workaround for this if you're already seeing it? I saw this a couple times this morning.
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
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)
Comment 9•13 years ago
|
||
(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•13 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 11•13 years ago
|
||
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•13 years ago
|
||
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•13 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.
Updated•13 years ago
|
Attachment #564793 -
Flags: review?(dtownsend) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #564793 -
Flags: approval-mozilla-beta?
Comment 14•13 years ago
|
||
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
Attachment #564793 -
Flags: approval-mozilla-beta?
Attachment #564793 -
Flags: approval-mozilla-beta+
Attachment #564793 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 15•13 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•13 years ago
|
Whiteboard: [land on mozilla-beta only]
Comment 16•13 years ago
|
||
Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/0c21f57bc10c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-firefox8:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
Backed out because this caused debug xpcshell orange:
https://hg.mozilla.org/releases/mozilla-beta/rev/33cce1d6ebc3
Assignee | ||
Comment 18•13 years ago
|
||
Here's the extra 8 characters needed to stop this breaking on debug.
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Attachment #564793 -
Attachment is obsolete: true
Comment 19•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
status-firefox8:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [land on mozilla-beta only]
Target Milestone: --- → mozilla8
Assignee | ||
Comment 20•13 years ago
|
||
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•13 years ago
|
Keywords: checkin-needed
Comment 21•13 years ago
|
||
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 → ---
Comment 22•13 years ago
|
||
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•13 years ago
|
||
Attachment #568920 -
Attachment is obsolete: true
Attachment #568940 -
Attachment is obsolete: true
Attachment #569193 -
Flags: review?(dtownsend)
Comment 24•13 years ago
|
||
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•13 years ago
|
Keywords: checkin-needed
Comment 25•13 years ago
|
||
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•13 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•13 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
Closed: 13 years ago → 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [land on mozilla-beta only]
Comment 28•13 years ago
|
||
(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•13 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".
Comment 30•13 years ago
|
||
(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•13 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.
Comment 32•13 years ago
|
||
Geoff, would we have to create a manual test or does the automated test cover everything?
Flags: in-testsuite+
Flags: in-litmus?
Assignee | ||
Comment 34•13 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•13 years ago
|
status-firefox9:
--- → unaffected
Updated•13 years ago
|
status-firefox8:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•