kangax / html-minifier

Javascript-based HTML compressor/minifier (with Node.js support)
http://kangax.github.io/html-minifier/
MIT License
4.94k stars 571 forks source link

Report error instead of freezing/hanging #332

Closed sabrehagen closed 8 years ago

sabrehagen commented 9 years ago

When a custom element is left unclosed, html-minifier will hang with no error reported. e.g.

<my-elem att="something" </my-elem>

This is different to when a standard html element is left unclosed. e.g.

<div att="something" </div>

which will give a parse error and report the offending statement.

zackiles commented 9 years ago

I can confirm this.

kangax commented 9 years ago

Marking as "feature" since it's all part of better error reporting that we'll get to at some point

kangax commented 9 years ago

Duplicate of #332

Meligy commented 9 years ago

@kangax this is not a duplicate of #332, because, um.. this is #332.

kangax commented 9 years ago

Oh, whoopsie, sorry :) Too many issues closed...

hwoarangzk commented 9 years ago

Will there be any update to solve this endless loop error? Really expect it coming soon

fregante commented 9 years ago

@hugowetterberg This is open source. Free. If you expect things to happen, do them yourself… or just fix your HTML, it's more desirable anyway.

hugowetterberg commented 9 years ago

So yeah, @bfred-it, I agree. But I guess that that was a typo and you actually meant @hwoarangzk

hwoarangzk commented 9 years ago

@bfred-it Maybe you don't know the problem detail. I write the correct html but got endless loop when using html-minifier, and I have to admit that I'm not able to fix it by modifying the html-minifier's code now. That's why I want to know whether this will be fixed in the future

hugowetterberg commented 9 years ago

@hwoarangzk as far as I can see this issue is about the minifier going into an infinite loop in the very specific case of an unclosed (malformed) custom element. So what you have encountered is either off topic for this issue OR your markup is malformed, there isn't really room for a middle ground.

If you're unable to contribute code that's completely ok, there are many kinds of contributions that you can make to open source projects. In this case you should verify that your markup actually is valid or at least doesn't contain the same mistake that this issue is about. Then try to remove markup until you're left with the minimal markup needed to reproduce the bug. Then file a separate issue that documents what you found.

@bfred-it reacted to the tone of your original comment, and rightly so. The details of the problem is simply not relevant as you just don't have any right to demand fixes from your peers. Even if that's not what you intended to do it's what your comment sounded like. Keep in mind that just as others should give you leeway because of the obvious language barriers you should try to be sensitive to when a comment has been misinterpreted. You really can't go wrong with generously apologising for unintended slights and backing up your actual arguments with concrete examples and reasoning.

Good luck and have fun!

hwoarangzk commented 9 years ago

@hugowetterberg I reported as a new issue #385, but kangax told me that it's duplicated with this issue, so I came in this #332 here. And I said the problem I came across is due to the ambiguous double-quotes even I escaped them. And I don't think that escaped quotes in html is malformed. Another, I'm from China and my English is not as good as you guys, so there maybe some misunderstanding in the post which you can tell me if there is anything I've done wrong. I hope you can understand :) All I want is just to solve the code issue.

fregante commented 9 years ago

@hwoarangzk I understand the issue, sorry if I was too harsh. Just so you know, "Really expect it coming soon" could have been "I hope it will be fixed soon," it sounds better :)

The code you posted in #385 is not valid, as you can test on http://validator.w3.org/nu/

hugowetterberg commented 9 years ago

Ok, it seems like this issue is becoming a catch-all for robust error reporting vs. just hanging. If the issue is that even properly escaped quotes trigger an inf loop then you should reduce your example to just that and not include the other templating stuff. Looking at your example it could just as well be the later markup that causes the issue:

<i><%= status == 2 ? "pass" : (status == 3 ? "fail" : (status == 1 ? "undealt" : "")) %></i>

That is not valid html, and running your template language through a html tool will probably not work.

So: reduce, reduce, and reduce, until you get rid of all ambiguity. And if the problem actually is properly escaped quotes you can ask for the issue to be re-opened.

Even those who have English as their first language offend each other unintentionally (all the time). But just a unreserved "Sorry, I didn't mean it that way" can get things like that cleared away quick & nice.

fregante commented 9 years ago

It looks like a recurring problem (with templates) that #382 could fix :-/

hwoarangzk commented 9 years ago

@bfred-it That's ok :) Out of blows friendship grows @hugowetterberg That's really my mistake not posted the actual html code with escaped double-quotes. I'll reopen that issue

emirotin commented 9 years ago

Encountered a similar issue - I had an equal sign missing between the attribute name and value:

<input type="number" max"{{ some angular interpolation }}">

johngrogg commented 9 years ago

Similarly, I accidentally had replaced a = with a - in an html attribute. I agree with @hugowetterberg that this all boils down to finding a way to catch infinite loops and report back at least some sort of generic error instead of just hanging.

Has anyone been able to track down where in the code the hanging is occurring? Is it lots of places depending on the mistake in the html code, or would there be one or two spots that are the culprits?

GriffinSauce commented 8 years ago

@hugowetterberg I'm not fully following you but are you suggesting that invalid html leading to 100% cpu hang is not an issue? Because in my opinion regardless of the input, be it expected, unexpected, malformed or invalid, a module should never hang.

hugowetterberg commented 8 years ago

@GriffinSauce Nope, I'm definitely not suggesting that. We don't want software to hang. Detecting infinite loops / halting state is, however, a pretty hard problem https://en.wikipedia.org/wiki/Halting_problem so that wasn't really what I argued for either @johngrogg. And programmatically detecting unreasonable resource usage is out of scope for the project. The normal thing to do after a bug like this would be to create a regression test that makes sure that that it doesn't resurface.

After I was pulled into this thread (by a mention-mistake :smile:) I just argued for the virtue of sticking to the topic of a thread, reducing any examples to the minimum required to reproduce a bug, and taking care not to sound like an entitled ass (because it's a very easy mistake to do).

But I'm actually not involved in this project, nor do I use it, I pretty much just responded because I wanted to help clearing up the communication issues.

alexlamsl commented 8 years ago

I think this is fixed by #510 - please reopen if that's not the case.