904464 - MXR no longer handling search strings with spaces (changed to +'s)

Status: RESOLVED FIXED

(Webtools Graveyard :: MXR, defect)

Description

[Edmund Wong (:ewong)]

Using http://mxr.mozilla.org/comm-central,

1) I enter "create a new account" in the search field. 
2) I click search.
3) The location : http://mxr.mozilla.org/comm-central/search?string=create%2Ba%2Bnew%2Baccount

but in the search for field, it shows "create+a+new+account".

I need to change the + to space in order to get a proper search.

Comment 1

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

server-ops: did something MXR-related change recently?

Comment 2

[Kendall Libby [:fubar] (he/him)]

Nothing in the MXR code has changed in I don't know how long, but the OS got an update a few weeks ago. It's possible that a pkg that mxr uses is behaving differently now.

Comment 3

[Justin Wood (:Callek)]

(In reply to Kendall Libby [:fubar] from comment #2)
> Nothing in the MXR code has changed in I don't know how long, but the OS got
> an update a few weeks ago. It's possible that a pkg that mxr uses is
> behaving differently now.

Perl packages change API and Behavior *all the time* when updated. Can you get a list of before and after for perl packages?

Comment 4

[Frank Wein [:mcsmurf]]

(In reply to Kendall Libby [:fubar] from comment #2)
> Nothing in the MXR code has changed in I don't know how long, but the OS got
> an update a few weeks ago. It's possible that a pkg that mxr uses is
> behaving differently now.

I see some changes at http://hg.mozilla.org/webtools/mxr/ lately, did these changes get deployed to mxr.mozilla.org?

Comment 5

[Justin Wood (:Callek)]

(In reply to Frank Wein [:mcsmurf] from comment #4)
> (In reply to Kendall Libby [:fubar] from comment #2)
> > Nothing in the MXR code has changed in I don't know how long, but the OS got
> > an update a few weeks ago. It's possible that a pkg that mxr uses is
> > behaving differently now.
> 
> I see some changes at http://hg.mozilla.org/webtools/mxr/ lately, did these
> changes get deployed to mxr.mozilla.org?

Indeed, and this looks very suspect since the symptom seems to be double-url-encode.

http://hg.mozilla.org/webtools/mxr/rev/60adab5f668c

Comment 6

[Kendall Libby [:fubar] (he/him)]

pulling in glob as he's the author of that patch (fixing a security issue from bug 628033)

Comment 7

[:glob ✱]

wow, so mxr does it's own decoding of the query_string, which explicitly double-encodes pluses.

it looks like there were issues with filenames with pluses (due to the lack of escaping in the header, which i fixed).  there's weird workarounds for that in various locations in the code.

eg. the filename in the directory head for http://mxr.mozilla.org/comm-central/source/mozilla/build/unix/stdc++compat/ is incorrect.


i'll see what i can do, but my ability to test this is somewhat limited.

Comment 8

[:glob ✱]

Attachment: 904464_1.patch

as best as i can tell, getting http_wash() to unescape + fixes this, and files with +'s in their name still work (eg comm-central/source/mozilla/build/unix/stdc++compat/ )

the issues with the path at the top of the mxr page wasn't related to my change, but i fixed it anyhow (it was using url escaping instead of html escaping).


if there's a mxr staging environment, this should be deployed there first for testing.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ping?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Indeed, ping. This is a quite bad regression.

Comment 13

[Erik Rose [:erik][:erikrose]]

Comment on attachment 795391 [details] [diff] [review]
904464_1.patch

Review of attachment 795391 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not set up to test this either, but the code looks saner than it was. I don't know of a staging env for MXR, but if we have one, let's put it there. If not, I lean toward deploying it anyway; we can try a few hits to make sure nothing obvious is broken and roll back if we discover something more subtle.

Comment 14

[Laura Thomson :laura]

Let's land and deploy - this is inconveniencing a lot of people.

Comment 15

[Laura Thomson :laura]

(FYI, I'm trying to find someone who can land and deploy this)

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 795391 [details] [diff] [review]
904464_1.patch

http://hg.mozilla.org/webtools/mxr/rev/6b92e127f403

Comment 17

[Corey Shields [:cshields]]

pushed this out and cleared the cache.

Comment 18

[Laura Thomson :laura]

Thanks to everybody that helped fix this!