591387 - Default theme shows as "incompatible" on the latest trunk nightly (also, version number wrapped with quotes)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Marcia Knous [:marcia]]

I noticed today when running Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4) Gecko/20100818 Firefox/4.0b4 that the default theme shows as incompatible. I don't recall this be the case yesterday.

Comment 1

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

Pushlog since yesterday:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75cc425bbaf0&tochange=e1d55bbd1d1d

Dave or Ben, could this be related to bug 586591?

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Pushlog since yesterday:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75cc425bbaf0&tochange=e1d55bbd1d1d
> 
> Dave or Ben, could this be related to bug 586591?

Can't imagine how it could be since she is running 4.0b4 which doesn't have that patch.

Comment 3

[Dave Townsend [:mossop]]

Marcia, can you attach a copy of extensions.sqlite from your profile folder.

Comment 4

[Marcia Knous [:marcia]]

Attachment: Screenshot of Addons dialog with error

Comment 5

[Marcia Knous [:marcia]]

I will attach the file in Comment 3 as well, but note I am running Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b5pre) Gecko/20100827 Minefield/4.0b5pre - I incorrectly pasted the wrong build ID in Comment 0.

Comment 6

[Marcia Knous [:marcia]]

Attachment: Copy of extensions.sqlite file

Comment 7

[Marcia Knous [:marcia]]

Attachment: Better version of sqlite file

Comment 8

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

Dave, something suspicious I see here is, that I'm not able to open the extensions.sqlite and addons.sqlite with the sqlite manager anymore. Other sqlite files can be opened without problems.

Comment 9

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

(In reply to comment #8)
> Dave, something suspicious I see here is, that I'm not able to open the
> extensions.sqlite and addons.sqlite with the sqlite manager anymore. Other
> sqlite files can be opened without problems.

Please scratch that. There was a process still running in the background.

I can see the same as Marcia only with todays build. The build from yesterday was fine.

Comment 10

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

The reason are the min and max version numbers:

<em:minVersion>"4.0b5pre"</em:minVersion>
<em:maxVersion>"4.0b5pre"</em:maxVersion> 

Somehow quotes have been added.

Comment 11

[Dave Townsend [:mossop]]

Good catch, looks like fallout from bug 581008

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

Comment 13

[[e-mail]]

Recommend you kick review to khuey, he's fast right now :)

Comment 15

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

Reverting summary (for the most part) so it is easier for people to find since it originally stated the symptom.

btw: the cause is the minVersion and maxVersion being wrapped in quotes which isn't technically invalid but it is incompatible.

Comment 16

[Axel Hecht [:Pike]]

FWIW, I suspect that this is fallout from the quoting changes that ted did to preprocessor.py.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #13)
> Recommend you kick review to khuey, he's fast right now :)

Not sure that's the reputation I want :-P

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

This patch should be a no-op (all it does is change the name of the variable from FIREFOX_VERSION to MOZ_UA_FIREFOX_VERSION so either there's something going on that I don't understand or it doesn't work.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, so this patch works because the makefiles for all the extensions are adding -DFIREFOX_VERSION=$(FIREFOX_VERSION) to the DEFINES variable.  This patch stops adding FIREFOX_VERSION to the DEFINES in configure, which lets those makefiles pick up the work.

What you should do is take the current patch, s/MOZ_UA_FIREFOX_VERSION/MOZ_APP_UA_FIREFOX_VERSION/ (kind of messy, but at least consistent), and then AC_DEFINE_UNQUOTED(FIREFOX_VERSION,$FIREFOX_VERSION) (with no quotes at all).  Then remove http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#46 http://mxr.mozilla.org/mozilla-central/source/extensions/widgetutils/Makefile.in#19 and http://mxr.mozilla.org/mozilla-central/source/extensions/metrics/Makefile.in#66

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Actually, on second though, I think MOZ_UA_FIREFOX_VERSION is preferable since that has nothing to do with the actual app we're building.

Comment 21

[Dão Gottwald [:dao]]

So what remains is this:

> AC_DEFINE_UNQUOTED(FIREFOX_VERSION,$FIREFOX_VERSION) (with no quotes at all). 
> Then remove
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#46
> http://mxr.mozilla.org/mozilla-central/source/extensions/widgetutils/Makefile.in#19
> and
> http://mxr.mozilla.org/mozilla-central/source/extensions/metrics/Makefile.in#66

... which is orthogonal to this bug?

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #21)
> So what remains is this:
> 
> > AC_DEFINE_UNQUOTED(FIREFOX_VERSION,$FIREFOX_VERSION) (with no quotes at all). 
> > Then remove
> > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#46
> > http://mxr.mozilla.org/mozilla-central/source/extensions/widgetutils/Makefile.in#19
> > and
> > http://mxr.mozilla.org/mozilla-central/source/extensions/metrics/Makefile.in#66
> 
> ... which is orthogonal to this bug?

Yeah, we can do that in a followup.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

This is fine for the moment, I'll file that followup.

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/7e70423b8749

Comment 25

[Robert Kaiser]

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

>diff -r 256514ea6d14 config/autoconf.mk.in
>+MOZ_UA_FIREFOX_VERSION = @FIREFOX_VERSION@

I'm pretty sure the right-hand side of this should be @MOZ_UA_FIREFOX_VERSION@ instead, as that's what the AC_SUBST is using.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #25)
> Comment on attachment 470040 [details] [diff] [review]
> patch
> 
> >diff -r 256514ea6d14 config/autoconf.mk.in
> >+MOZ_UA_FIREFOX_VERSION = @FIREFOX_VERSION@
> 
> I'm pretty sure the right-hand side of this should be @MOZ_UA_FIREFOX_VERSION@
> instead, as that's what the AC_SUBST is using.

It's more complicated than that, but it doesn't matter since nothing is actually using that in a makefile.  This is fairly hacky, I'm going to clean it up in bug 591611.

Comment 27

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

Should be fairly simple to add a mochitest to verify that the default theme's target application minVersion / maxVersion equals the current application's version to prevent this from slipping by in the future

Comment 30

[Axel Hecht [:Pike]]

This doesn't seem to be fixed for the langpacks, at least.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> This doesn't seem to be fixed for the langpacks, at least.

Please file a bug.

Comment 32

[Axel Hecht [:Pike]]

There have been two bugs resolved as DUPE of this.

Comment 33

[Dão Gottwald [:dao]]

I reopened that bug. This one is fixed.

Comment 34

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre