Closed Bug 1093034 Opened 10 years ago Closed 9 years ago

JavaScript error: chrome://mozapps/content/extensions/extensions.xml, line 1304: TypeError: addonType is undefined

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 + fixed
firefox37 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

STR:

1. Open about:addons
2. Select "Get Add-ons"
3. In the search box on the top right, enter "ChatZilla" and hit enter

ER:
bunch of search results with install buttons

AR:
the language pack items have every button under the sun, and for each instance of those, there's this error in my console:

JavaScript error: chrome://mozapps/content/extensions/extensions.xml, line 1304: TypeError: addonType is undefined
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Not broken on beta
Last good revision: 02fbb6ada9cb
First bad revision: bc7deafdac4b
Pushlog:
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=02fbb6ada9cb&tochange=bc7deafdac4b

I guess it's caused by bug 1056490, it adds case for langpack (but not sure).
  https://hg.mozilla.org/mozilla-central/rev/4b0ba1775012

I'll continue further bisecting...
mozregression tells me it's bug 1056490.
Blocks: 1056490
[Tracking Requested - why for this release]:
user-facing regression that we shouldn't ship

Basically, the add-on type should have been added to the addon manager as well, because now the code here:

http://hg.mozilla.org/mozilla-central/annotate/5dde8ea48fef/toolkit/mozapps/extensions/content/extensions.xml#l1308

breaks. There may be other cases, too.

Ideally this should get tests so we don't break it again.
Attachment #8531798 - Flags: review?(dtownsend+bugmail)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I don't think this needs a test, but feel free to convince me otherwise... :-)
Iteration: --- → 37.1
Points: --- → 2
Flags: in-testsuite? → in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
(In reply to :Gijs Kruitbosch from comment #6)
> I don't think this needs a test, but feel free to convince me otherwise...
> :-)

Why is that?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #6)
> I don't think this needs a test, but feel free to convince me otherwise...
> :-)

In comment 4 you said "Ideally this should get tests so we don't break it again."...
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #8)
> (In reply to :Gijs Kruitbosch from comment #6)
> > I don't think this needs a test, but feel free to convince me otherwise...
> > :-)
> 
> In comment 4 you said "Ideally this should get tests so we don't break it
> again."...

Right, because I didn't think this would get fixed the way it did. I discussed the fix with Mossop face to face - because the search UI (with this patch) filters out anything it doesn't understand, new additions to the list in AddonRepository.jsm / AMO wouldn't require any more changes there, ie I think this is less likely to regress than I thought it would be when I wrote comment 4.

Of course, positive changes (ie adding things we do want to see in search results) would 'regress' this if we forgot to update it - but that's not really testable (we could add a dummy type entry, but that wouldn't "test" for us not forgetting to add the real stuff if we ever do so).
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8531798 [details] [diff] [review]
fix add-on manager to cope with magical new add-on types,

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

I think it would be good to have a test here, or at least extend one of the existing ones. It should be pretty straightforward to add some unknown type results into one of the tests, it should make the test fail without this patch and pass with it.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +1097,2 @@
>              case 6:
> +              addon.type = "locale";

Did you ever find out what these two types were?
Attachment #8531798 - Flags: review?(dtownsend+bugmail) → review+
(In reply to Dave Townsend [:mossop] (Out till December 1st) from comment #10)
> ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
> @@ +1097,2 @@
> >              case 6:
> > +              addon.type = "locale";
> 
> Did you ever find out what these two types were?

I've only ever seen type "langpack" (5) but no langpack-addon items.
From #amo just now:

[11:40:41]	Gijs	Can someone link me to a public addon of type 6 ("langpack-addon") ?
[11:40:55]	clouserw 	Gijs: I don't think there are any
[11:41:06]	Gijs	clouserw: is that type dead or something?
[11:41:09]	clouserw	 it was more of an idea that never went anywhere
[11:41:16]	clouserw	 afaik
[11:41:23]	Gijs	clouserw: it's interesting because there are such things on AMO, but they're marked 5 ("langpack")
[11:41:35]	clouserw	 those are not the same
[11:41:37]	Gijs	e.g. https://addons.mozilla.org/en-US/seamonkey/addon/chatzilla-zh-cn-language-pack/
[11:41:46]	Gijs	clouserw: what's the theoretical difference?
[11:42:02]	clouserw	 ah, that would be the same
[11:42:14]	clouserw	 but since it was never supported it's not marked correctly
[11:42:35]	clouserw	 langpacks are for the browser, langpack-addon was supposed to be a langpack for an add-on (like what you linked to)


so I think treating them the same is OK for now - we can definitely install the things currently marked 'langpack' even if they were marked 'langpack-addon', and they provide l10n stuff. And realistically, right now AMO will never send us results of this kind.
Yeah sounds like they are basically the same from Firefox's standpoint so fine to treat them the same.
Iteration: 37.1 → 37.2
It's bad enough when you write a patch and you decide you don't need a test, and then your reviewer quite rightfully tells you that this shouldn't be hard and that maybe it's worthwhile.

Then it gets worse because it turns out that actually, the tests don't work the way you think they do, and so it takes several hours to add the test you wanted.

Then you find out that the test actually passed before your patch, too, so clearly the test is no good.

The worst is that then, after you manage to make the test fail without the patch, you confidently rerun it with the patch... and find that it keeps failing.

Long story short: the code here was actually wrong (or rather, not good enough).

Not only is the conditional I added in the previous patch inverted (it's missing a ! ), it wasn't added in enough places, and the reason I didn't see that before now is that the manual testing I was doing was wallpaper-fixed by the other hunk (of making langpacks be treated as 'locale').

This patch started from scratch based on the now properly failing testcase (also in the patch), and seems to work on the tests I ran locally. Try push to see what that says:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=695f3b18d238
Attachment #8534723 - Flags: review?(dtownsend+bugmail)
Attachment #8531798 - Attachment is obsolete: true
Comment on attachment 8534723 [details] [diff] [review]
fix add-on manager to cope with magical new add-on types,

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

(I got curious, after seeing discussion in #fx-team)

::: toolkit/mozapps/extensions/test/browser/browser_searching.xml
@@ +42,5 @@
> +        <link>http://example.com/creator.html</link>
> +      </author>
> +    </authors>
> +    <status id='4'>Public</status>
> +    <summary>Addon with uninstallable type shouldn't be visible in search</summary>

"uninstallable" is perhaps not the best word here, given it sounds like it can be uninstalled.
Attachment #8534723 - Flags: review?(dtownsend+bugmail) → review+
So the failing xpcshell tests can be fixed either like this, or (I guess) by making the results that test_AddonRepository.js and test_AddonRepository_cache.js use (ie http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/data/test_AddonRepository_cache.xml ) specify a type where they are expected to be returned by the code here. I'm actually unsure what the correct fix is - Dave/Blair, can you weigh in? :-)
Attachment #8534991 - Flags: review?(dtownsend+bugmail)
Attachment #8534991 - Flags: review?(bmcbride)
Comment on attachment 8534991 [details] [diff] [review]
followup: keep addons with no type specified,

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

(In reply to :Gijs Kruitbosch from comment #16)
> Created attachment 8534991 [details] [diff] [review]
> followup: keep addons with no type specified,
> 
> So the failing xpcshell tests can be fixed either like this, or (I guess) by
> making the results that test_AddonRepository.js and
> test_AddonRepository_cache.js use (ie
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/data/test_AddonRepository_cache.xml ) specify a type where
> they are expected to be returned by the code here. I'm actually unsure what
> the correct fix is - Dave/Blair, can you weigh in? :-)

So we have a test verifying that the search API will return add-ons of unknown types. That is pretty much inconsistent with the change we're making here so either:

1. The API shouldn't return any types we don't know about, i.e. we have to add a type to that test case or remove it from the test entirely.
2. The API should return everything including unknown types and we have to move the filtering code you've written to wherever in the frontend we build the search results to display.

I don't know why we'd let the API return a result for an add-on that we don't know the type of so I'm inclined to go with 1 and change the testcase.
Attachment #8534991 - Flags: review?(dtownsend+bugmail)
Attachment #8534991 - Flags: review?(bmcbride)
Attachment #8534991 - Flags: review-
(In reply to Dave Townsend [:mossop] from comment #17)
> Comment on attachment 8534991 [details] [diff] [review]
> followup: keep addons with no type specified,
> 
> Review of attachment 8534991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Gijs Kruitbosch from comment #16)
> > Created attachment 8534991 [details] [diff] [review]
> > followup: keep addons with no type specified,
> > 
> > So the failing xpcshell tests can be fixed either like this, or (I guess) by
> > making the results that test_AddonRepository.js and
> > test_AddonRepository_cache.js use (ie
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > test/xpcshell/data/test_AddonRepository_cache.xml ) specify a type where
> > they are expected to be returned by the code here. I'm actually unsure what
> > the correct fix is - Dave/Blair, can you weigh in? :-)
> 
> So we have a test verifying that the search API will return add-ons of
> unknown types. That is pretty much inconsistent with the change we're making
> here so either:
> 
> 1. The API shouldn't return any types we don't know about, i.e. we have to
> add a type to that test case or remove it from the test entirely.
> 2. The API should return everything including unknown types and we have to
> move the filtering code you've written to wherever in the frontend we build
> the search results to display.
> 
> I don't know why we'd let the API return a result for an add-on that we
> don't know the type of so I'm inclined to go with 1 and change the testcase.

So I'm trying to adapt the testcase, but it seems that the code in test_AddonRepository_cache.js also tests getAddonsByIds [0], which it explicitly passes the ID of an add-on that has the wrong type, which AIUI is already installed? (I'm assuming from the header on [1] that these are installed)

What should happen in this case? I'm uncomfortable making the code lie about installed add-ons, so it seems like they should be returned.

In general, I'm not sure why the tests of the local add-on cache are affected by changes to search - is there a local cache for non-installed add-on info as well, which is then used for tests of search functionality?


Aside, IMO the whole thing is hairy enough that we shouldn't land this on beta at this point because it won't get enough testing over Christmas and the assorted December/January holidays. I'm going to prepare a backout of bug 1056490 for beta because I think the regression isn't shippable, and we have had the error messages for a long time now - one release more isn't going to hurt anyone.

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js#640 and further.
[1] https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonManager
Flags: needinfo?(dtownsend)
Flags: needinfo?(bmcbride)
Attached patch Backout on betaSplinter Review
Attachment #8538445 - Flags: review?(dtownsend)
Attachment #8538445 - Flags: review?(dtownsend) → review+
(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Dave Townsend [:mossop] from comment #17)
> > Comment on attachment 8534991 [details] [diff] [review]
> > followup: keep addons with no type specified,
> > 
> > Review of attachment 8534991 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to :Gijs Kruitbosch from comment #16)
> > > Created attachment 8534991 [details] [diff] [review]
> > > followup: keep addons with no type specified,
> > > 
> > > So the failing xpcshell tests can be fixed either like this, or (I guess) by
> > > making the results that test_AddonRepository.js and
> > > test_AddonRepository_cache.js use (ie
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > > test/xpcshell/data/test_AddonRepository_cache.xml ) specify a type where
> > > they are expected to be returned by the code here. I'm actually unsure what
> > > the correct fix is - Dave/Blair, can you weigh in? :-)
> > 
> > So we have a test verifying that the search API will return add-ons of
> > unknown types. That is pretty much inconsistent with the change we're making
> > here so either:
> > 
> > 1. The API shouldn't return any types we don't know about, i.e. we have to
> > add a type to that test case or remove it from the test entirely.
> > 2. The API should return everything including unknown types and we have to
> > move the filtering code you've written to wherever in the frontend we build
> > the search results to display.
> > 
> > I don't know why we'd let the API return a result for an add-on that we
> > don't know the type of so I'm inclined to go with 1 and change the testcase.
> 
> So I'm trying to adapt the testcase, but it seems that the code in
> test_AddonRepository_cache.js also tests getAddonsByIds [0], which it
> explicitly passes the ID of an add-on that has the wrong type, which AIUI is
> already installed? (I'm assuming from the header on [1] that these are
> installed)
> 
> What should happen in this case? I'm uncomfortable making the code lie about
> installed add-ons, so it seems like they should be returned.

So this is a little crazy, the API result says this add-on is of type 9999, the actual add-on installed is of type 4 (theme). I don't know why that was done, too long ago. Let's just change the type of that add-on in the XML to theme (<type id="2">Theme</type>).

> In general, I'm not sure why the tests of the local add-on cache are
> affected by changes to search - is there a local cache for non-installed
> add-on info as well, which is then used for tests of search functionality?

The search API is used for two things. Searching in the UI is the obvious one. The unobvious bit is where daily we "search" AMO for data on all the installed add-ons and cache it locally to supplement what we got from an add-ons install.rdf.
Flags: needinfo?(dtownsend)
Flags: needinfo?(bmcbride)
Erik, why do those bugs depend on this one?
Flags: needinfo?(evold)
(In reply to :Gijs Kruitbosch from comment #21)
> Erik, why do those bugs depend on this one?

I noticed a similar error, I want to confirm this isn't a blocker later.
Flags: needinfo?(evold)
Comment on attachment 8538445 [details] [diff] [review]
Backout on beta

Approval Request Comment
[Feature/regressing bug #]: bug 1056490
[User impact if declined]: broken UI in addon manager for some add-ons
[Describe test coverage new/current, TBPL]: extensive testing, but didn't catch the issue I'm backing this out for, so probably won't have any effect when I back it out, either...
[Risks and why]: very low, straight backout.
[String/UUID change made/needed]: no.
Attachment #8538445 - Flags: approval-mozilla-beta?
Attached patch Test fixesSplinter Review
Attachment #8534991 - Attachment is obsolete: true
Attachment #8538718 - Flags: review?(dtownsend)
Attachment #8538718 - Flags: review?(dtownsend) → review+
Ah so the error that I was see was different:

Error: "NS_ERROR_FAILURE: Invalid view: detail" {file: "chrome://mozapps/content/extensions/extensions.js" line: 719}]

Sorry.
No longer blocks: 1028773, 1111107
https://hg.mozilla.org/mozilla-central/rev/de0d7a6eb13f
https://hg.mozilla.org/mozilla-central/rev/d5cbc65d37b0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8538445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8534723 [details] [diff] [review]
fix add-on manager to cope with magical new add-on types,

Approval Request Comment
[Feature/regressing bug #]: bug 1056490
[User impact if declined]: broken add-on manager UI for some add-ons
[Describe test coverage new/current, TBPL]: has extensive tests (which had to be slightly modified for these changes)
[Risks and why]: pretty limited considering the amount of testing
[String/UUID change made/needed]: no
Attachment #8534723 - Flags: approval-mozilla-aurora?
Attachment #8534723 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.