Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong format on Internet Explorer #104

Closed
gotjosh opened this issue Apr 29, 2015 · 12 comments · Fixed by #105
Closed

Wrong format on Internet Explorer #104

gotjosh opened this issue Apr 29, 2015 · 12 comments · Fixed by #105
Assignees
Milestone

Comments

@gotjosh
Copy link

gotjosh commented Apr 29, 2015

Howdy fellow lotus lovers!

I noticed that two of my Lotus Production apps won't allow Internet Explorer access. They would immediately throw a 500 error when trying to access them. After some digging, these are my findings:

The HTTP_ACCEPT header that IE (short for Internet Explorer from now on) is coming with; is text/html, application/xhtml+xml, image/jxr, */* when checking self.format inside an controller action we can see that this translates to :'123'. Weird right?

After digging around in lotus/controller I noticed this particular method inside mime.rb

def accepts
  unless accept == DEFAULT_ACCEPT
    ::Rack::Utils.best_q_match(accept, ::Rack::Mime::MIME_TYPES.values)
  end
end

Since accept is equal to HTTP_HEADER the result from Rack's best_q_match is "application/vnd.lotus-1-2-3" (more info on this here) which is what is ultimatetly determining the not-so-cool :'123' format.

SIDENOTE

I took a few side steps onto Rack to check out this function and this is the result from q_values for this particular string:

[["text/html", 1.0], ["application/xhtml+xml", 1.0], ["image/jxr", 1.0], ["*/*", 1.0]]

Seems weird that the quality of all of them is 1.0 considering that text/html should be the one to be respected, in particular */* is the one being extrapolated to "application/vnd.lotus-1-2-3", I know that our very own @jodosha patched this function and added a test case that involves this particular string so I believe something broke in Rack along the way. You can take a look at @jodosha's patch here

I want to help out here, so my question is, is this something that should be patched/fixed in Lotus or completely delegate this to Rack? It's believe is quite bad for us (Lotus) that IE is broken out of the box.

@jodosha jodosha self-assigned this May 4, 2015
@jodosha jodosha added the bug label May 4, 2015
@jodosha jodosha added this to the v0.4.1 milestone May 4, 2015
@jodosha
Copy link
Member

jodosha commented May 4, 2015

@gotjosh Hello and thanks for reporting this problem. I'm going to investigate and decide what to do. It's a blurry line between Rack and Lotus::Controller 😸

jodosha added a commit that referenced this issue May 4, 2015
@jodosha
Copy link
Member

jodosha commented May 4, 2015

@gotjosh I'm a confused. First of all, I've introduced a test to verify the failure that you've reported, but it's passing without changing a line in the production code (see 31e929b). From this perspective looks like a "non-bug".

However, wrong format causes a wrong Content-Type response header sent to the browser. How this can cause a server side error?

You have referenced a Rack patch, are you using 1.6.0?

Can you please share the stack trace? Thank you very much.

@jodosha jodosha added the waiting label May 4, 2015
@jodosha
Copy link
Member

jodosha commented May 4, 2015

@gotjosh There is one problem here: the build fails on Ruby 2.0+ (2.2 excluded). https://travis-ci.org/lotus/controller/builds/61126011

I suspect you're on 2.0 or 2.1, correct?

@gotjosh
Copy link
Author

gotjosh commented May 4, 2015

Hi @jodosha! It's a quite blurry line if you ask me, that's why I wanted to ask first. Allow me to respond in order:

  1. Yes, indeed a wrong format causes a wrong Content-Type and therefore the template that you're going to render is not found which causes the "server side" error during rendering phase.

  2. I am using rack 1.6, here's the stack trace:

stack-trace

  1. Yep, I'm on 2.1.2 and the build matrix seems about right. In you take a look it's stating that the format is 123 which comes from the lotus interpretation of application/vnd.lotus-1-2-3 which is what rack is returning. Seems weird that this feature is Ruby related.

@jodosha
Copy link
Member

jodosha commented May 4, 2015

This is related to #59

@gotjosh
Copy link
Author

gotjosh commented May 4, 2015

Ohhh! I completely missed that rack patch inside lotus/controller 😢

@gotjosh
Copy link
Author

gotjosh commented May 4, 2015

However the patch states is only for Rack 1.5 or below, I'm currently on Rack 1.6.

@jodosha
Copy link
Member

jodosha commented May 4, 2015

However the patch states is only for Rack 1.5 or below, I'm currently on Rack 1.6.

True, but I just discovered that Rack.release is broken on 1.6.0: it still returns "1.5". See rack/rack#751 and rack/rack#777

That patch was always applied 😱 That means Rack 1.6 still has a bug in the MIME type handling.

The last commit above should fix the build for all the supported Rubies.

@gotjosh
Copy link
Author

gotjosh commented May 4, 2015

Fantastic! :shipit:

@jodosha
Copy link
Member

jodosha commented May 4, 2015

@gotjosh Can you please verify that master now fixes your app? If so, I'll be happy to do a patch release for this.

@headius
Copy link

headius commented May 29, 2015

The logic you've applied here is fragile and will probably break again in the future...because it is patching around differences in sort implementations.

In jruby/jruby#3004, we have an issue caused by the way our sort implementation handles equivalent entries. A test case in Lotus uses an accept specification with two equivalently-weighed mime types, and then expects that they'll be ordered the same across all Ruby implementations. Unluckily for us, JRuby differs: jruby/jruby#3004 (comment)

It's also interesting that prior to 19345cc, Rubinius was treated as an odd man out.

There's a fundamental issue here with the way the weighted sorting is done; you're going to see sort implementations vary over time and across Ruby implementations, and the way they sort equivalent entries will cause this to break again and again. I would suggest that you add some additional weighting based on the position in the accept list, so equivalent entries won't be quite so equivalent.

@jodosha
Copy link
Member

jodosha commented May 30, 2015

@headius THB, I'm not proud of that patch. Another poor quality indicator is the high churn in this area.

That being said, we wanted to borrow from Rack::Utils as much as possible. As they should cover most of the edge cases that come from browsers quirks. Until few months ago (can't recall if pre MRI 2.2 or Rack 1.6), the order of the returned entries was following their order of apparence.

It suddenly broke and we had to reverse. 😢 It deserves a solid implementation, I'm gonna to rewrite it and send a PR to the Rack team.

Thanks for having a look at our builds. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants