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)
Toolkit
Add-ons Manager
Tracking
()
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
5.13 KB,
patch
|
Unfocused
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
mossop
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•10 years ago
|
||
Not broken on beta
status-firefox34:
--- → unaffected
status-firefox35:
--- → ?
status-firefox36:
--- → affected
Keywords: regression,
regressionwindow-wanted
Comment 2•10 years ago
|
||
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...
Assignee | ||
Comment 4•10 years ago
|
||
[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.
tracking-firefox35:
--- → ?
Updated•10 years ago
|
tracking-firefox36:
--- → +
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8531798 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•9 years ago
|
||
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
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
(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."...
Assignee | ||
Comment 9•9 years ago
|
||
(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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
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.
Comment 13•9 years ago
|
||
Yeah sounds like they are basically the same from Firefox's standpoint so fine to treat them the same.
Updated•9 years ago
|
Iteration: 37.1 → 37.2
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8531798 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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-
Assignee | ||
Comment 18•9 years ago
|
||
(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)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8538445 -
Flags: review?(dtownsend)
Updated•9 years ago
|
Attachment #8538445 -
Flags: review?(dtownsend) → review+
Comment 20•9 years ago
|
||
(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)
Updated•9 years ago
|
Flags: needinfo?(bmcbride)
Assignee | ||
Comment 21•9 years ago
|
||
Erik, why do those bugs depend on this one?
Flags: needinfo?(evold)
Comment 22•9 years ago
|
||
(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)
Assignee | ||
Comment 23•9 years ago
|
||
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?
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8534991 -
Attachment is obsolete: true
Attachment #8538718 -
Flags: review?(dtownsend)
Assignee | ||
Comment 25•9 years ago
|
||
remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=355b2bad99bd
Updated•9 years ago
|
Attachment #8538718 -
Flags: review?(dtownsend) → review+
Comment 26•9 years ago
|
||
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.
Assignee | ||
Comment 27•9 years ago
|
||
With the trivial fix for the failure on try: remote: https://hg.mozilla.org/integration/fx-team/rev/de0d7a6eb13f remote: https://hg.mozilla.org/integration/fx-team/rev/d5cbc65d37b0
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
Updated•9 years ago
|
Attachment #8538445 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/ce333283811b
status-firefox37:
--- → fixed
Assignee | ||
Comment 30•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8534723 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 31•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3dc1923089f https://hg.mozilla.org/releases/mozilla-aurora/rev/1facdf833813
You need to log in
before you can comment on or make changes to this bug.
Description
•