291792 - "undefined" folder created because installDroppedInFiles doesn't check for return value of getIDForLocation

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Nickolay_Ponomarev]

This code is wrong (installDroppedInFiles function, nsExtensionManager.js):
--
var id = location.getIDForLocation(entry);
actualItems[id] = entry;
--

Should be 
--
var id = location.getIDForLocation(entry);
if(!id)
  continue;
actualItems[id] = entry;
--

The symptoms are: put a "{3f0da09b-c1ab-40c5-8d7f-53f475ac3fe8}~" (i.e. not a
GUID) file pointing to a correct extension location in profile/extensions.
Firefox creates an "undefined" folder in profile/extensions and says in the JS
console it tries to register item with ID=undefined.

Comment 1

[Benjamin Smedberg]

Patches welcome!

Comment 2

[Nickolay_Ponomarev]

dang. Scratch that. Steps to reproduce are simpler: just create a non-GUID named
folder in extensions/ with install.rdf in it.

Why not just read the ID from install.rdf? That would allow for simpler folder
names, as some people prefer. If you want to save time on parsing install.rdf
you could create a plaintext file with GUID inside the folder and then first try
to read from it.

I'd make a patch if I had a tree.
Gavin? ;-)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch (untested)

Comment 4

[Nickolay_Ponomarev]

Similar fix appears to be included in attachment 181795 [details] [diff] [review] (second patch in bug
291831).

Comment 5

[Darin Grimm]

Based on comment #4 and the fact that the second patch in 291831 has landed, is
this bug fixed?

Comment 6

[Nickolay_Ponomarev]

Comment on attachment 181815 [details] [diff] [review]
Patch (untested)

Seems so, but I don't have a recent build. If anyone can confirm this is fixed,
please mark it as such. Removing obsolete review request, sorry for wasting
your time, Gavin.

Comment 7

[Nickolay_Ponomarev]

Nope, not fixed.

Comment 8

[Anbo Motohiko]

(In reply to comment #3)
> Created an attachment (id=181815) [edit]
> Patch (untested)
> 

Gavin, your approach is correct. Would you please attach new patch?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: Unbitrotted

This should be OK, since getIDForLocation clearly can return undefined, but I'm
not able to actually test it, nor have I taken the time to really understand
any of this code.

Comment 10

[Asa Dotzler [:asa]]

robert, can you look at this patch and see if it can be updated?

Comment 11

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

Gavin or Nickolay (if you are around) - could you check if you are able to still
reproduce this bug? I just tried and it did not create an undefined folder for
me when using the steps in comment #0. Thanks - if I can find the time I'll
verify it also by inspecting the code.

Comment 12

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

Comment on attachment 182594 [details] [diff] [review]
Unbitrotted

Benjamin - this patch from gavin.sharp does fix the problem as stated in
comment #2 by skipping invalid id's and it still applies.

Comment 13

[timeless]

Comment on attachment 182594 [details] [diff] [review]
Unbitrotted

mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in 	1.118