Deface/JRuby closes head tag leaving head elements within the body #84

swrobel · 2013-05-02T21:44:51Z

This is similar to the behavior reported in spree/spree#2633

Test this issue with:

https://github.com/spree/spree/tree/jruby
https://github.com/spree/deface/tree/master
jruby-1.7.3

Look at the HTML source of the index page or run it through http://validator.w3.org/ to see all of the errors this generates. First few lines of the doc as a sample:

<!DOCTYPE html>
<!--[if lt IE 7 ]> <html class="ie ie6"  lang="en"> <![endif]--><!--[if IE 7 ]>    <html class="ie ie7"  lang="en"> <![endif]--><!--[if IE 8 ]>    <html class="ie ie8"  lang="en"> <![endif]--><!--[if IE 9 ]>    <html class="ie ie9"  lang="en"> <![endif]--><!--[if gt IE 9]><!--><html lang="en">
<!--<![endif]-->
<head data-hook="inside_head">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<!-- Spree Analytics has not been initialized correctly -->

</head>
<body>
<meta charset="utf-8">
<title>Spree Demo Site</title>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type" />
<meta content="width=device-width, initial-scale=1.0, maximum-scale=1" name="viewport">
<meta content="spree, demo" name="keywords" />
<meta content="Spree demo site" name="description" />

The text was updated successfully, but these errors were encountered:

jumph4x · 2013-05-11T05:41:48Z

@swrobel @BDQ This may not be related to the cross-referenced bug.

This will happen if you're streaming responses out with Deface enabled. It will try parsing incomplete and thereby invalid markup and insert shit left and right.

Found this out preparing my talk for SpreeConf actually.

To verify, disable streaming temporarily or just launch with a server that doesn't support chunked responses like the vendored WEBrick.

Cheers

BDQ · 2013-05-13T09:19:47Z

@jumph4x very interesting, thanks for posting.

@swrobel are you using streaming respones?

swrobel · 2013-05-14T15:25:18Z

@jumph4x @BDQ I just tried starting my jruby server with rails s webrick (was using puma) and I'm seeing the same behavior, so that didn't fix it for me.

@jumph4x can you provide more info about what exactly your scenario is? Are you talking about running jruby? I'm using streaming successfully w/ MRI & unicorn without this deface issue. It only crops up on jruby and doesn't seem to be dependent on the webserver (puma & webrick both exhibit it)

@BDQ have you been able to repro? I'm running a super stock setup here and seeing this, so I'm hoping you've been able to test on your end.

jumph4x · 2013-05-14T16:06:27Z

@swrobel @BDQ I lied. The symptoms will persist even with a non-streaming server.

It will curb all benefits of streaming, but rails will still try to flush the buffer (meaning a premature pass through libxml), only difference is that it will be held at WEBrick until the full response is received.

I reproduced in development. I'd consider this an app-level concern. I stream in production and I precompile deface. See recent fix.

swrobel · 2013-05-14T16:09:53Z

Interesting, so I'm not crazy! @jumph4x if I understand correctly, the temporary solution is to precompile deface when using jruby until this is resolved?

jumph4x · 2013-05-14T16:14:03Z

(I wasn't using Jruby)

Basically, the way things stand, I don't think there is any say we have in how libxml treats chunked responses. And they leave the domain of the app before ever being combined in some circumstances (your and my production, it seems).

So while I accept invalid markup for development environment with deface compiling into memory (default), I certainly precompile all deface views in production and have no problems. So yes? 🎉

BDQ · 2013-05-14T19:25:47Z

Thinking about it I don't see how Deface could affect Streaming responses (or vice versa). Deface hooks into, and "overrides" the view at compile time - i.e. when file view is pulled from disk and just before it's passed to Erubis for compilation. So the Deface changes are made before any of the streaming logic could possible take effect, ergo it's not related.

@swrobel - I'm sure I'll be able to reproduce, and I have an inkling I know where the issue is. I'm just swamped with SpreeConf prep right now and won't get to it for a while. I appreciate your patience.

swrobel · 2013-05-14T19:32:00Z

@BDQ no problem, just wanted to make sure I wasn't leading you on a wild goose chase

jumph4x · 2013-05-14T20:06:52Z

@BDQ and @swrobel No pressure whatsoever, I know we're all busy, but going to leave this here for when you find a moment.

Here is what I think is happening.

config.deface.disabled = false
https://github.com/spree/deface/blob/master/lib/deface/railtie.rb#L55 is required at railtie loadtime
https://github.com/spree/deface/blob/master/lib/deface/action_view_extensions.rb#L8 is run each time an ActionView::Resolver instantiates a new ActionView::Template
https://github.com/spree/deface/blob/master/lib/deface/applicator.rb#L6 shows where the above #apply method comes from
https://github.com/spree/deface/blob/master/lib/deface/applicator.rb#L19 is run if any overrides are found
https://github.com/spree/deface/blob/master/lib/deface/parser.rb#L103-109 show how regardless, the document will pass through Nokogiri

@BDQ 's logic would be sound if all of this somehow only affected ActionView::Template.render at https://github.com/spree/deface/blob/master/lib/deface/action_view_extensions.rb#L25 but it actually affects template instantiation, which needless to say, does NOT just occur at compile time.

Perhaps I am missing something, but let's wait until the SpreeConf craze is over and we'll sort it.

BDQ · 2013-05-15T08:05:09Z

@jumph4x - Thanks for all the details, the render method on action_view_extensions.rb#L25 is a very edge feature (that we only ever used on StoreWorks) normally in production has no effect.

As as test you could comment that out and see if the issue persists (as I expect it would). This method can probably be dropped now.

jumph4x · 2013-05-15T08:18:12Z

What I meant to articulate is that it is the #initialize not the #render that is ultimately responsible for this, if you re-read just ignore the last 2 paragraphs. I should type less... I just cause confusion 😆

Tested the things I traced above: commented out the initialization alias chain in https://github.com/spree/deface/blob/master/lib/deface/action_view_extensions.rb#L2-18. Output:

<!DOCTYPE html>
<html>
<head>
  <link href="assets0.fcpstores.com" rel="dns-prefetch" />
...
</head>
<body>
...

Correct output. Put the #initialize chain back, but commented out the render one as in https://github.com/spree/deface/blob/master/lib/deface/action_view_extensions.rb#L20-38:

<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<link href="assets0.fcpstores.com" rel="dns-prefetch" />
...

Specifically, notice the meta http-equiv="Content-Type" content="text/html; charset=UTF-8" inserted by deface.

So I still hold this is due to the way Deface forced ActionView:: Template to instantiate, thereby causing the Applicator class to apply HTML parsing by way of Nokogiri/libxml.

jumph4x · 2013-05-15T08:20:14Z

For anyone looking to test this one for themselves:

Ensure Deface is enabled
Stream something from the controller in the render {:action => something, :stream => true}
bundle show deface
cd into above directory and manually comment out sections as I outlined above
Remember to fully restart the rails server when manually modified bundled gems' files

BDQ · 2013-05-15T09:19:45Z

@jumph4x - this is beginning to make sense. You do have overrides defined for the layout in question?

Deface (or more specifically the Applicator) doesn't pass the source through Nokogiri unless there's overrides defined for it.

https://github.com/spree/deface/blob/master/lib/deface/applicator.rb#L13

jumph4x · 2013-05-15T09:26:02Z

Yup, all default Spree installations do, assuming we're all using and/or overriding spree_application.html.erb :)

[1] pry(main)> Rails.application.config.deface.overrides
=> #<Deface::Environment::Overrides:0x00000006d1f410
 @all=
  {:"spree/layouts/spree_application"=>
    {"add_analytics_header"=>
      #<Deface::Override:0x00000007c78628
       @args=
        {:virtual_path=>"spree/layouts/spree_application",
         :name=>"add_analytics_header",
         :insert_bottom=>"head",
         :partial=>"spree/analytics/header",
         :original=>"6f23c8af6e863d0499835c00b3f2763cb98e1d75",
         :updated_at=>1368609774.9760637,
         :railtie_class=>"Spree::Dash::Engine"}>},

(:

swrobel · 2013-05-21T19:11:43Z

Per @jumph4x's advice, I tried precompiling deface under jruby. Thanks to Denis & @BDQ for walking me through the finer points of deface @ spreeconf. It seems that precompilation does not solve this, so streaming is definitely not at fault under jruby. See below for code from app/compiled_views/spree/layouts/spree_application.html.erb

<!DOCTYPE html >
<!--[if lt IE 7 ]> <html class="ie ie6"  lang="<%= I18n.locale %>"> <![endif]--><!--[if IE 7 ]>    <html class="ie ie7"  lang="<%= I18n.locale %>"> <![endif]--><!--[if IE 8 ]>    <html class="ie ie8"  lang="<%= I18n.locale %>"> <![endif]--><!--[if IE 9 ]>    <html class="ie ie9"  lang="<%= I18n.locale %>"> <![endif]--><!--[if gt IE 9]><!--><html lang="<%= I18n.locale %>"><!--<![endif]-->
  <head data-hook="inside_head">
    <% if Spree::Dash::Config.configured? %>
  <script id="analytics" type="text/javascript">
    var jirafe= <%= raw analytics_tags.to_json %>;
    (function(){
    var d=document,g=d.createElement('script'),s=d.getElementsByTagName('script')[0];
    g.type='text/javascript',g.defer=g.async=true;g.src=d.location.protocol+'//c.jirafe.com/jirafe.js';
    s.parentNode.insertBefore(g, s);
    })();
  </script>
<% else %>
  <!-- Spree Analytics has not been initialized correctly -->
<% end %></head><body><%= render :partial => 'spree/shared/head' %>

jumph4x · 2013-05-22T09:18:16Z

Looks like this is probably something to do with either JRuby or parsing logic branching in libxml itself, as @BDQ mentioned.

The only last idea I have is to try and remove conditional IE <html> declarations because I think I see an instance of questionably valid html such as <!--> but that's a stretch.

swrobel · 2013-05-22T16:51:54Z

@BDQ as promised, here's my Gemfile for jruby testing: https://gist.github.com/swrobel/7b43e4ade4db9567f5a7

mrbanzai · 2013-05-28T20:49:23Z

As this appears to be caused by a "bug" in libxml 2.9, and has been worked around in Nokogiri 1.6.0-rc1, any chance of getting the Nokogiri version dependency updated in the gemspec?

swrobel · 2013-05-28T20:50:57Z

@mrbanzai I'm pretty sure jruby doesn't use libxml because java has xml parsing builtin...

mrbanzai · 2013-05-28T21:01:23Z

@swrobel Ah, I may be hijacking an issue then; my apologies. I'm seeing the same symptoms with MRI using Nokogiri 1.5.9 / libxml 2.9 as mentioned in spree/spree#2633 (referenced by the description). I'll create a separate issue regarding the dependency version, if necessary.

BDQ · 2013-05-29T10:22:38Z

I did try bumping to nokogiri to 1.6.0.rc1 when it was released but as there's no jruby version the bundle install fails when using jruby.

It looks like whatever fix that's in 1.6.0.rc1 is not needed on jruby so they are in no rush to release a java version of it until it's final.

I'm open to suggestions (and especially pull requests) that allow different dependencies per platform (I did a quick google and it doesn't appear to be trivial).

BDQ · 2013-05-29T13:24:58Z

@swrobel - I've figured out what's going on here:

So layouts/spree_application.html.erb has the following markup:

  <head data-hook="inside_head">
    <%= render :partial => 'spree/shared/head' %>
  </head>

Deface temporarily converts this (before passing to Nokogiri) into :

  <head data-hook="inside_head">
     <code erb-loud> render :partial => 'spree/shared/head' </code>
  </head>

libxml on MRI is fine with a "code" tag being inside the "head" tag, but on jruby (and whatever java parser it uses) it decides the "code" can't be in "head" and moves into to body.

swrobel · 2013-05-29T17:25:15Z

@BDQ nice discovery! I don't know if W3Schools is to be believed, but according to this page there are a limited number of tags allowed within head (trying to read the W3C specs makes my head spin). Perhaps the Java parser is actually doing it right?

Any ideas for a fix/workaround?

BDQ · 2013-05-30T09:35:42Z

A possible workaround would be to:

Add an override to replace contents of head using 'spree/shared/head'

Deface::Override.new(virtual_path: 'spree/layouts/spree_application',
replace_contents: 'head',
partial: 'spree/shared/head')

Add a second override to remove the bad render 'spree/shared/head' from the body

Deface::Override.new(virtual_path: 'spree/layouts/spree_application',
remove: "body code:contains('spree/shared/head')")

Both are untested.

jumph4x · 2013-07-22T22:51:42Z

About to cross reference something I found that is OS-lib-level, on MRI.

This changes the ERB template parser to use `<erb>` rather than `<code>` to designate code blocks in a template. For example: <%= render template: 'foo/bar' %> Used to parse as: <code erb-loud> render template: 'foo/bar' </code> But not parses as: <erb erb-loud> render template: 'foo/bar' </erb> This fixes a number of problems that result from using `<code>`: * There may be </code> tags in the original ERB, which would be turned into `%>` (Fixes spree#101) * Any <code> tags in the <head> of a document would be moved to the body, since <code> is a real HTML tag, and isn't allowed in the head. This happens in nokogiri under jRuby or libXML 2.9.0 (Fixes spree#84, Fixes spree#100)

swrobel mentioned this issue May 2, 2013

Jruby: output not properly converted back to erb #74

Closed

jumph4x mentioned this issue Jul 22, 2013

LibXML2 2.9.0+ problems #100

Closed

jhawthorn mentioned this issue Sep 3, 2013

Use <erb> instead of <code> when parsing templates #105

Merged

BDQ closed this as completed in 415422f Sep 4, 2013

International mentioned this issue Sep 8, 2013

Override breaks javascript load order jipiboily/monologue-markdown#5

Open

tramuntanal mentioned this issue Jun 2, 2023

Add Google Tag Manager in admin header ConsorciAOC/Decidim-Catalunya#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deface/JRuby closes head tag leaving head elements within the body #84

Deface/JRuby closes head tag leaving head elements within the body #84

swrobel commented May 2, 2013

jumph4x commented May 11, 2013

BDQ commented May 13, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

BDQ commented May 14, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

BDQ commented May 15, 2013

jumph4x commented May 15, 2013

jumph4x commented May 15, 2013

BDQ commented May 15, 2013

jumph4x commented May 15, 2013

swrobel commented May 21, 2013

jumph4x commented May 22, 2013

swrobel commented May 22, 2013

mrbanzai commented May 28, 2013

swrobel commented May 28, 2013

mrbanzai commented May 28, 2013

BDQ commented May 29, 2013

BDQ commented May 29, 2013

swrobel commented May 29, 2013

BDQ commented May 30, 2013

jumph4x commented Jul 22, 2013

Deface/JRuby closes head tag leaving head elements within the body #84

Deface/JRuby closes head tag leaving head elements within the body #84

Comments

swrobel commented May 2, 2013

jumph4x commented May 11, 2013

BDQ commented May 13, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

BDQ commented May 14, 2013

swrobel commented May 14, 2013

jumph4x commented May 14, 2013

BDQ commented May 15, 2013

jumph4x commented May 15, 2013

jumph4x commented May 15, 2013

BDQ commented May 15, 2013

jumph4x commented May 15, 2013

swrobel commented May 21, 2013

jumph4x commented May 22, 2013

swrobel commented May 22, 2013

mrbanzai commented May 28, 2013

swrobel commented May 28, 2013

mrbanzai commented May 28, 2013

BDQ commented May 29, 2013

BDQ commented May 29, 2013

swrobel commented May 29, 2013

BDQ commented May 30, 2013

jumph4x commented Jul 22, 2013