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

Javascript error with stories #295

Closed
swanson opened this issue Jan 7, 2014 · 19 comments
Closed

Javascript error with stories #295

swanson opened this issue Jan 7, 2014 · 19 comments
Labels

Comments

@swanson
Copy link
Collaborator

swanson commented Jan 7, 2014

Something in here is causing a Uncaught SyntaxError: Unexpected token ILLEGAL error.

http://pastebin.com/T1MFyvdX (too large for GH issues)

@mpraglowski
Copy link

I have the same issue - any solution or workaround ?
The message is: SyntaxError: Unexpected EOF

@Koronen
Copy link
Contributor

Koronen commented Jan 13, 2014

I've managed to reproduce the error using only the following snippet.

new AppView(new StoryList).loadData([{"body":"\u003Ca href=\"http://bit.ly/1dQNk8n%E2%80%A8%E2%80%A8\" target=\"_blank\"\u003Ehttp://bit.ly/1dQNk8n

\u003C/a\u003E"}]);

The error seems to originate in the following, though I'm not sure why. It probably has something to do with character encoding.

"

\u003C/a\u003E"

@swanson
Copy link
Collaborator Author

swanson commented Jan 13, 2014

Workaround - mark all as read :) Use the archive to look at what you missed.

I had a feeling it was related to a strange character.

@jhass
Copy link
Contributor

jhass commented Jan 16, 2014

+1

My workaround is adding .gsub(/[^[:print:]]/, '') to https://github.com/swanson/stringer/blob/master/app/views/js/stories.js.erb#L7

@swanson
Copy link
Collaborator Author

swanson commented Feb 24, 2014

Let's apply this fix - but on the model, not in the view.

@Koronen
Copy link
Contributor

Koronen commented Feb 24, 2014



\u003C and \u003E are printable characters (< and > respectively) so they should not be affected by the RegExp above. @MrZyx have you been having issues with non-printable characters (like control characters) in your feeds?

@jhass
Copy link
Contributor

jhass commented Feb 24, 2014

Hm, I can't believe I had that only a month ago, it feels so much longer. Anyway I don't remember what sort of input caused this for me, only that I had the same error and that that snippet fixed it for me (without marking all stories as read or anything like that).

@Koronen
Copy link
Contributor

Koronen commented Feb 24, 2014

Okay, that's unfortunate. Stripping non-printable characters does still sound like a sensible thing to do, though.

@Koronen
Copy link
Contributor

Koronen commented Feb 24, 2014

It seems that non-printable characters are already taken care of by the StoryRepository.sanitize method. I added the following test and it was green before I made any modifications to the production code.

it "strips non-printable characters" do
  result = StoryRepository.sanitize("foo\u0007\u001Cbar")
  result.should eq "foobar"
end

@swanson
Copy link
Collaborator Author

swanson commented Feb 24, 2014

So do we need special handling for > < & etc then?

Maybe the issue is in backbone?

@jhass
Copy link
Contributor

jhass commented Feb 24, 2014

From a quick look it seems to only be applied to the content, not the title though.

@kjell
Copy link

kjell commented Mar 10, 2014

This has happened to me a few times. Firefox says SyntaxError: unterminated string literal.

Today the problem character was <
> (<\U+FFE2\U+FFA8> 8232, Hex 2028, Octal 20050).

(Edit, 13 days later: It happened again)

@swanson
Copy link
Collaborator Author

swanson commented Mar 28, 2014

I think I've got this fixed now - the problem was unprintable characters as @MrZyx pointed out weeks ago :)

The funny part about unprintable characters is that...well, they don't print! So it was hard to see that there was one actually there. I did a manual "bisect" approach to pinpoint the offending character from this failing case:

new AppView(new StoryList).loadData([{"body":"\u003Ca href=\"http://bit.ly/1dQNk8n%E2%80%A8%E2%80%A8\" target=\"_blank\"\u003Ehttp://bit.ly/1dQNk8n

\u003C/a\u003E"}]);

and found that there was a \U+FFE2\U+FFA8 tacked on after QNk8n. How it got there in the first place? A mystery for another day.

Please update your Stringer installations (instructions are in the README) and let me know if you encounter this again. Thanks everyone for helping us track down this annoying issue!

@jhass
Copy link
Contributor

jhass commented Mar 28, 2014

Thanks @swanson

I'd still consider applying sanitize to the tile and maybe even the URL too though. Or am I missing something?

@swanson
Copy link
Collaborator Author

swanson commented Mar 28, 2014

We can certainly do that - just wanted to get this issue with the body resolved first.

@ghostwords
Copy link

71199cc doesn't fix displaying already-fetched stories. I was getting a SyntaxError: unterminated string literal from a U+2028 in a story body, the reason being that U+2028 is valid in JSON but not JavaScript strings.

The following change fixed my problem:

diff --git a/app/views/js/stories.js.erb b/app/views/js/stories.js.erb
index f937cce..b990ea2 100644
--- a/app/views/js/stories.js.erb
+++ b/app/views/js/stories.js.erb
@@ -4,7 +4,7 @@
   $(document).ready(function() {
     var Stories = new StoryList;
     var StoryApp = new AppView(Stories);
-    StoryApp.loadData(<%= stories.to_json %>);
+    StoryApp.loadData(<%= stories.to_json.gsub(/[\u2028\u2029]/, '') %>);

     Mousetrap.bind("j", function() {
       StoryApp.moveCursorDown();

Surprised that JSON libs don't do JavaScript-context escaping by default.

Possibly related Ruby on Rails issue.

@swanson
Copy link
Collaborator Author

swanson commented Mar 30, 2014

@ghostwords Yes - that is true. My general philosophy has been to catch these kind of invalid characters when they come into the system (i.e. when the feed is parsed). This has the advantage of only needing to happen in one place, not on every case where we display the data.

The fix I would advise for existing stories would be a simple one-off script to fix the unprintable characters in your database. Something like:

Story.all.each do |story|
  story.body = story.body.gsub(/[^[:print:]]/, '')
  story.save
end

It's certainly not ideal - but that should resolve your issue without making more code changes.

@ghostwords
Copy link

Ah, sounds good. How's this?

diff --git a/Rakefile b/Rakefile
index d2983e7..e392db2 100644
--- a/Rakefile
+++ b/Rakefile
@@ -63,6 +63,14 @@ task :cleanup_old_stories, :number_of_days do |t, args|
   RemoveOldStories.remove!(args[:number_of_days].to_i)
 end

+desc "Sanitize all stories."
+task :sanitize do
+  Story.all.each do |story|
+    story.body = StoryRepository.sanitize(story.body)
+    story.save
+  end
+end
+
 desc "Start server and serve JavaScript test suite at /test"
 task :test_js do
   require_relative "./spec/javascript/test_controller"

@Koronen
Copy link
Contributor

Koronen commented Mar 31, 2014

I don't think this is something we need to commit into the repo. As @swanson said, it's a one-time fix.

swanson pushed a commit that referenced this issue Apr 23, 2014
We were too aggressive with our fixing of #295. [:print:] is catching \r\n, so let's explicitly fix the weird JSON unicode chars (this aligns with the rack fix as well: https://github.com/rack/rack-contrib/pull/37/files).
swanson pushed a commit that referenced this issue Apr 23, 2014
Add migration to correct unicode issues that were resolved by #295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants