785326 - "504 Gateway Time-out" error when adding a new review containing a long word

Status: VERIFIED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P3)

Description

[Cristian Boldan]

Steps to reproduce:
1. Load Marketplace homepage https://marketplace-dev.allizom.org/
2. Click on a free app to load details page
3. Click on "Submit a review" button
4. Add a review containing a long word (ex. a word formed of 40 characters)

Expected results:
User is redirected to the reviews page and the review is added.

Actual results:
"504 Gateway Time-out" error is displayed on a blank page.

Notes/Issues:
Verified in FF17 (Win 7).
Screencast for this issue http://screencast.com/t/hygg6Mf8po

Comment 1

[Potch [:potch]]

I can totally reproduce this, much to my unending dismay.

Comment 2

[Wil Clouser [:clouserw]]

Reviews are broken right now so I can't verify (bug 798347) but this is definitely weird.

Comment 3

[Wil Clouser [:clouserw]]

(In reply to Wil Clouser [:clouserw] from comment #2)
> Reviews are broken right now so I can't verify (bug 798347) but this is
> definitely weird.

This is still a thing

Comment 4

[Matt Basta [:basta]]

I can reproduce this bug for reviews, but not for replies. That means that the code most likely lives in mkt.ratings.forms.

Comment 5

[Matt Basta [:basta]]

If I'm not mistaken, this is caused here:

https://github.com/mozilla/zamboni/blob/master/apps/reviews/forms.py#L41

The regex that we're using to test if links exist is very, very greedy. Basically it seems to get stuck in an exceedingly long pattern that tries to match groups of strings over and over and over, but only finds out once it gets to the end that it's not a URL and tries matching from the next character.

If this is the case, my vote is to test only for URLs that have an ((ht|f)tps?:)?// prefix or a numeric IP address OR to have the pattern updated to only match URLs that bleach will linkifiy automatically. As it stands, this regex seems to be too inefficient.

Can someone confirm this locally?

Comment 6

[Rob Hudson [:robhudson]]

I can confirm it takes longer than it should. I used the string, "this is a very long word: supercalifragilisticexpeallidocious" and that regex alone took 8 seconds to search the string.

Comment 7

[Wil Clouser [:clouserw]]

Good catch, basta.  Importing/using bleach is a good idea.

Comment 8

[Matt Basta [:basta]]

I'll do you one better:

https://github.com/mozilla/zamboni/commit/d939d3ca8ae7a51c814f13abd797399a31017987

This makes the current test more generic (it'll catch more TLDs than bleach) and it's at least an order of magnitude faster. We'd also have to write some icky stuff, as it turns out, since bleach's regex pattern parses only links with a protocol on them. This saves a lot of hassle and code has been shipped. :)

Comment 9

[Cristian Boldan]

Verified as fixed in https://marketplace-dev.allizom.org/ on FF19(Win 7)
Postfix screencast http://screencast.com/t/CZssbEuBGf
Closing bug.