505177 - Cannot install experimental add-ons in certain circumstances

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P1)

Description

[Kohei Yoshino [:kohei]]

An add-on author reported us that he couldn't install experimental add-ons.
I confirmed the bug when:

* The add-on is labeled as an experimental (sandboxed) add-on
* The add-on has an EULA
* A user is not logged-in
* A user tries to install the add-on via its details page (not listing page)

e.g. https://preview.addons.mozilla.org/en-US/firefox/addon/12421

1. Check 'Let me install this experimental add-on' box
2. Click 'Add to Firefox' button to see the EULA
3. Check 'Let me install this experimental add-on' again
4. Click 'Accept and Install' button
5. You'll see 'invalid file hash' error when install

The problem happens at Step 3. You can find duplicated '?confirmed' query strings in the Firefox location bar before Step 4.

Comment 1

[Kohei Yoshino [:kohei]]

(In reply to comment #0)
> The problem happens at Step 3. You can find duplicated '?confirmed' query
> strings in the Firefox location bar before Step 4.

Nope -- The problem happens at Step 1. You can find invalid query strings '?src=addondetail' and '?confirmed'. before Step 2.

Comment 2

[Kohei Yoshino [:kohei]]

Attachment: patch

Comment 3

[Kohei Yoshino [:kohei]]

Attachment: patch, shortened

Comment 4

[fcp]

This sounds like the same issue as bug 488092 (but not a duplicate).  Either the fix of 488092 was incomplete or something broke it again.

Comment 5

[fcp]

(In reply to comment #3)
> Created an attachment (id=389437) [details]
> patch, shortened

I do not know PHP, but if I understand what the patched code does, testing only /\?/ should be sufficient and make much more sense.

Comment 6

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

Kohei, you probably want to ask Alex for review.

Comment 7

[Kohei Yoshino [:kohei]]

Comment on attachment 389437 [details] [diff] [review]
patch, shortened

The patch should match
https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/xxxxx/yyyyy?src=addondetail
and the link will be
https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/xxxxx/yyyyy?src=addondetail&confirmed

Comment 8

[Wil Clouser [:clouserw]]

This is a regression.  Since buchanae has the r? assigning to him and marking for 5.0.8

Comment 11

[fcp]

(In reply to comment #7)
> The patch should match
> https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/xxxxx/yyyyy?src=addondetail
> and the link will be
> https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/xxxxx/yyyyy?src=addondetail&confirmed

Kohei, I am not sure whether this is a reply to my comment #5 or not.  If it is, I still think that the test against “collection_id” is unnecessary.

(Ignore my silly comment about PHP....  After I wrote it, I realized that the patch was not against the code written in PHP.)

Comment 12

[Bob Davies]

My users are complaining they are still unable to update, any idea when this patch is expected to go live please?

Comment 13

[Alex Buchanan [:abuchanan]]

Hi Bob,

The 5.0.8 milestone is scheduled for code freeze on 7/29 and launch on 8/5.  

I'm working towards those dates.  I've been trying to properly review Kohei's patch, but I have run into issues with my test setup of AMO.

Sorry for the inconvenience to your users and you.  Thanks for being patient.  I'm hoping I can get this figured out today.

Comment 14

[Alex Buchanan [:abuchanan]]

Comment on attachment 389437 [details] [diff] [review]
patch, shortened

ok, after getting my dev. setup issues worked out, I applied this patch and tried to install addon 12421, and it still didn't work.

I'm working on a patch now.

Comment 15

[Alex Buchanan [:abuchanan]]

(In reply to comment #14)
> (From update of attachment 389437 [details] [diff] [review])
> ok, after getting my dev. setup issues worked out, I applied this patch and
> tried to install addon 12421, and it still didn't work.
> 
> I'm working on a patch now.


FTR, I just realized this patch applies to webroot/js/addons.js, while I believe we're still working off webroot/js/amo2009/addons.js

I copied over the file, and the problem still exists though.  Damn that amo2009 gets me every time :)

Comment 16

[Alex Buchanan [:abuchanan]]

Comment on attachment 389437 [details] [diff] [review]
patch, shortened

After some confusion and digging, it turns out this patch solved the problem as intended.

The issue I was seeing had to do with a bad addon hash.  Once I recalc'd the hash, everything seems to work.

Comment 17

[Alex Buchanan [:abuchanan]]

patch applied to webroot/js/amo2009/addons.js in r39718.

For testing, you'll probably need to upload your own sandboxed add-on to preview.amo, as the files for add-on #12421 don't exist.

Comment 18

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

I can't click on the "Let me install this expiremental add-on" checkbox, using Firefox 3.5.1 on Mac/Win, on https://preview.addons.mozilla.org/en-US/firefox/addon/8585.

The weird thing is that it works fine on the search-results page: https://preview.addons.mozilla.org/en-US/firefox/search?q=stephen3000&cat=all

Comment 19

[Alex Buchanan [:abuchanan]]

The issue is that the preview image is overlapping the checkbox,

http://www.grabup.com/uploads/da7b2fa67e1d739c1706dd4cc0c9a47c.png

Comment 20

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

Attachment: Screenshot of preview image overlapping the checkbox (Firebug)

Attaching locally so we don't lose the image.

Comment 21

[Alex Buchanan [:abuchanan]]

I'm pretty sure this was caused by bug 504579 and r30301.

rdoherty, could your mad CSS skills help us fix this, please?

Comment 22

[Milos Dinic [:Milos]]

Do you guys want me to fix this if :rdoherty is busy?

Comment 23

[Milos Dinic [:Milos]]

Attachment: patch for fixing css that caused overlaping

This limits width of a image to a 205px(which is used on all other AMO plublic pages), and now you can check that checkbox to install addon.

Comment 24

[Milos Dinic [:Milos]]

Just to mention, this fix won`t work for IE browsers.

Comment 25

[Fred Wenzel [:wenzel]]

Comment on attachment 391365 [details] [diff] [review]
patch for fixing css that caused overlaping

That works, thanks! I checked this in to r48153.

Alex, is this bug fixed now? Wanna close it? Thanks.

Comment 26

[Wil Clouser [:clouserw]]

This is blocking code freeze so if it's fixed let's get it closed.  Sorry I can't test right now.

Comment 27

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

Verified FIXED:

[1] URL after enabling the download button was https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/12421/55363?src=addondetail&confirmed
[2] URL after clicking on the download button and going through the EULA was https://preview.addons.mozilla.org/en-US/firefox/downloads/file/55363/clifton__s_creative_design_toolbar-1.5.42.0-fx.xpi?confirmed
[3] Even though I got a -261 error, I think (based on a lot of past experience), that that's just preview
[4] Tested other browsers, and they all just give us the "Download Now" button correctly