470652 - file attached to review request is missing

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Download the file to review from https://addons.mozilla.org/de/editors/review/58020 (it's in the update queue). You get a HTML page about file not found.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Any update on this?

Comment 2

[Wil Clouser [:clouserw]]

Looks like for the most part people are nominating these for admin review.  It's still broken for admins but at least it gets it out of the general review queue.  Thanks for pointing out the bug.  I'll drop it into our current milestone with the expectation to bump it to 5.0.3.  This way it will stay on the radar and if someone gets to it sooner we can squeeze it in.

Comment 3

[u278084]

I found a way to reproduce the "file not found" error on my local box. It's pretty ugly/stupid, so I suspect that there might be more than 1 way to do this. Since the addon is in the update queue, you need to start with an addon that is already public :

STR:
1. Login. Using the new developer tools, click add a new version.
2. Upload a new version of the extension (I just changed the version number of an addon)
3. Go back to Versions and Files, and click on the version you updated
4. Click "Add New File", and upload the same xpi you did in step #2. (in the update queue, you'll now see the same extension twice in the queue).
5. Go back to Versions and Files, and delete the first xpi you uploaded in step #2.
6. In the update queue, you'll see the extension only mentioned once. But trying to download or view contents will give you a file not found error.

Whew!

Comment 4

[Fred Wenzel [:wenzel]]

(In reply to comment #3)
> 4. Click "Add New File", and upload the same xpi you did in step #2. (in the
> update queue, you'll now see the same extension twice in the queue).
> 5. Go back to Versions and Files, and delete the first xpi you uploaded in step
> #2.

Between these two steps it looks like the file names are the same so the file upload does not fail or change the file name so two versions refer to the same file. When you delete one of them, the one and only file is removed, leaving the second version with a "dangling pointer".

Cesar: Did you want to have a crack at this, or do you want me to figure it out?

Comment 5

[u278084]

(In reply to comment #4)
> Cesar: Did you want to have a crack at this, or do you want me to figure it
> out?

Sure, I can take it. I think you pretty much figured it out, but I'll have a crack at the patch.

Comment 6

[u278084]

Attachment: v1

Turns out the file name is constructed with every submission to be addonname-version-apps-platform.(xpi|jar). So even if the filenames were different when submitted, this bug should still be present. I worked around this by attaching the date timestamp to the end of the filename in YYYYMMDDHHmm format.

Comment 7

[Fred Wenzel [:wenzel]]

Hm, that'll change all (future) AMO file names. Maybe it'd be less invasive to add a number at the end only if necessary? The vast majority of add-ons will not get into this conflict anyway.

Comment 8

[u278084]

(In reply to comment #7)
> Hm, that'll change all (future) AMO file names. Maybe it'd be less invasive to
> add a number at the end only if necessary? The vast majority of add-ons will
> not get into this conflict anyway.

That's doable. I thought the solution was more clever, but it is more taxing on the eyes with something like 200902261925. Something else we can also do is just explicitly require developers to delete the old addon first.

Comment 9

[Fred Wenzel [:wenzel]]

(In reply to comment #8)
> Something else we can also do is
> just explicitly require developers to delete the old addon first.

I was thinking the same thing and I tried to find an old bug about that (somewhere in the back of my head I thought we had one about "do not allow two versions with the same version number") but I failed finding it. Hmmm.

Comment 10

[u278084]

Just to be clear on this, because I realize that I wasn't on the STR step #2. You have to upload the addon as an additional file and not from the regular "Upload a new version" link or "Submit Add-on" menuitem.

So it must be uploaded from
https://addons.mozilla.org/en-US/developers/versions/addfile/54833
and not
https://addons.mozilla.org/en-US/developers/versions/add/9067

Sorry for any confusion. If you try and upload as a new version, than it will try and stop you.

Comment 11

[Wil Clouser [:clouserw]]

Is the goal here clear or is there an open question?

Comment 12

[Fred Wenzel [:wenzel]]

From what it seems, adding the same file twice to the same version leads to a filename collision. I think the question is currently, how this should be fixed correctly.

Cesar: Does this appear when you add two files to the same add-on version, but both with target platform "all"? In other words, should the behavior be blocked?

Comment 13

[Nick Nguyen [:osunick]]

Shouldn't the new file simply replace the old if it's the same filename?

Comment 14

[u278084]

Attachment: screenshots are worth 10^100 words

(In reply to comment #12)
> Cesar: Does this appear when you add two files to the same add-on version, but
> both with target platform "all"? In other words, should the behavior be
> blocked?
Yep. This will appear any time the generated (not uploaded) filename is constructed the same. So if you uploaded two files both for the BSD platform, but one will be replaced.

I think the right thing to do is to block developers from uploading additional files that will collide with existing ones. Something like :

"The file you have uploaded has the same name, version, and platform of an existing file. You must delete the old file first before uploading the another one."

(In reply to comment #13)
> Shouldn't the new file simply replace the old if it's the same filename?

That's the problem we have now. I attached a screenshot to show the problem better. Disregard the size differences. If I delete one of those garbage-1.0-fx.xpi files, then the rest will return as file not found.

Comment 15

[Nick Nguyen [:osunick]]

Cesar:  I concur.  If the file is different they should increment the number.

Also, would this be an example of a file that is affected by this bug:

https://addons.mozilla.org/en-US/firefox/files/browse/44826/1

Comment 16

[u278084]

Attachment: v2

Stop the developer from accidentally overwriting the file.

I hope the message will be clear enough when I say "delete the file x first". Considering you have to go through the edit versions page to upload the file, it would be kinda hard to miss. But surprises do happen.

Comment 17

[Fred Wenzel [:wenzel]]

Comment on attachment 365730 [details] [diff] [review]
v2

Looks good!

Comment 18

[Fred Wenzel [:wenzel]]

Cesar, will you be able to check this in today, or do you want me to do it? Tonight is the 5.0.3 code freeze.

One minor tweak: Please start your localizer comment in the .po file with "#.", not just "#". We recently needed to change that in order to make the gettext tools play well with us and push the comments to the other locales properly.

Comment 19

[u278084]

Committed with the tweak. In r23024

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

I see this:

"Oops! There seems to be a problem with this file...

devcp_error_file_exists

Please correct this problem and upload your file again." on https://preview.addons.mozilla.org/en-US/developers/versions/addfile/57204, when I try re-uploading the same add-on file.

Do we need an Apache restart to pick up the gettext() changes on preview.AMO?

Comment 21

[Fred Wenzel [:wenzel]]

Cesar should probably also add in-line fallback by using ___() instead of _() -- sorry I missed that when reviewing.

Cesar, do you know what I am talking about ;-) and want to add it?

Comment 22

[u278084]

(In reply to comment #21)
> Cesar should probably also add in-line fallback by using ___() instead of _()
> -- sorry I missed that when reviewing.
> 
> Cesar, do you know what I am talking about ;-) and want to add it?

Sure. Does it need review or can I just check that in?

Comment 23

[Fred Wenzel [:wenzel]]

(In reply to comment #22)
> Sure. Does it need review or can I just check that in?

Just check it in, but feel free to mention the revision here, for good measure.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Reopening until the fallback changes from comment 21 have landed.

Comment 25

[u278084]

Used modified gettext in r23083. Also, somehow the en_US locale string was set as a fuzzy string. So I fixed that. Hopefully it'll be fixed now :)

Comment 26

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post 2nd-fix screenshot

Nit: it's "add-ons", rather than "addons".

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED

Comment 28

[Wil Clouser [:clouserw]]

(In reply to comment #26)
> Created an attachment (id=366716) [details]
> Post 2nd-fix screenshot
> 
> Nit: it's "add-ons", rather than "addons".

Fixed in r23091

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #28)
> (In reply to comment #26)
> > Created an attachment (id=366716) [details] [details]
> > Post 2nd-fix screenshot
> > 
> > Nit: it's "add-ons", rather than "addons".
> 
> Fixed in r23091

It just showed up on preview; that fix is also verified fixed.