Prototype breaks jsChannel, and thus H

[csillag]

When trying to annotate some text here:

http://sportgeza.hu/sport/vivovb/2013/08/10/iszonyu_fajdalmas_vereseg_a_romanoktol/

I get this:

This happens with both the extension, and locally with latest dev.

Very interesting, I have never seen anything like that.

Update: The root cause of the problem will be handled by this upstream bug: openannotation/annotator#251 This ticket is now about how to avoid the situations when our code gets in a brain-dead, half-working zombie state like this, because of some problem in an annotator plugin. We should add shielding for that.

[csillag]

The problem seems to be caused that in same cases, when transferring it from the host frame to the sidebar, jschannel messes up the target field of the annotation: before the transmission, it's an array, but after the transmission, it arrives as a string. (It looks like the string expression of the original array.)

Probably has to do with some character encoding issues...

[csillag]

No, it's not just character encoding. Still diggin'.

[csillag]

As @gergely-ujvari has discovered, this is probably caused by openannotation/annotator#251
However, I feel that there should be at lest two or three layers of error-checking shielding us from that problem.

(Failing in an extremely roundabout way because of some initialization problem is not nice.)

[edsu]

I don't know if this helps, but I spent some time in the debugger, and can see that the annotation is flattened by the time that it makes it to Annotation.save in controllers.js which gets called when the user clicks to save an annotation in the editor. If you want to see it, set a breakpoint for line 751 in controllers.js, look at $scope.model.$modelValue.document.link and notice how it is a string instead of a list. For some reason I am suspecting the bridge or jschannel -- but I have no evidence for that. I need to spend more time in the debugger I guess.

[csillag]

When I have added some debug output inside bridge, to the two ends of the showEditor call, then I saw that when it is called in the host frame, targets is a list, but when it arrives in the sidebar (still inside bridge), it is already a string.

So it is happening inside jsChannel... but somehow it is still triggered by what has happened a long time ago, in the Document plugin? Sounds a bit crazy...

[edsu]

I forgot to mention that this is reproducible now, using the url @gergely-ujvari found for openannotation/annotator#251 ... which I see now is referenced at the top of this issue too 😊

[csillag]

That link is also referenced in the OP of the current issue.

[gergely-ujvari]

Actually I was using the url @csillag mentioned at the top of this issue.

[edsu]

@csillag Have you tried disabling the Document plugin to see if it still happens? If you are right the problem should go away.

[csillag]

No, I have not. I am leaving this bug to you; I have plenty of others to take care of :)

[edsu]

On one side of jschannel https://github.com/hypothesis/h/blob/develop/h/js/host.coffee#L192 I can see that metadata.link is a list, and then on the other side https://github.com/hypothesis/h/blob/develop/h/js/services.coffee#L372 it is flattened into a string. So the problem seems to be in jschannel.

[csillag]

Yes, I have seen exactly the same thing: data structure on one end of jsChannel, string on the other end. The question is, what is triggering this? Because this does not happen on most of the pages! There must be something special to trigger this.

Either some prior event which messes up the brain of jsChannel, or something in the data actually being transferred...

[edsu]

I'm going to code up a unit test for jschannel using the JavaScript object we are passing across, and see if I can replicate the problem.

[edsu]

In copying the data from the Chrome console to a text file I noticed that:

JSON.stringify({foo: ["bar", "baz"]})

evaluates to:

"{"foo":"[\"bar\", \"baz\"]"}"

which if you then parse:

JSON.parse(JSON.stringify({foo: ["bar", "baz"]}))

the list gets flattened into a string?!?

{foo: "["bar", "baz"]"}

Am I going crazy, or is JSON.stringify not working properly? Another wrinkle is that in another tab say at http://hypothes.is JSON.stringify works fine.

"{"foo":["bar","baz"]}"

Stringify only seems to fail at http://sportgeza.hu/sport/vivovb/2013/08/10/iszonyu_fajdalmas_vereseg_a_romanoktol/ (and possibly elsewhere?)

I'm confused 😬

[edsu]

I created a standalone HTML page that has all the sportgeza.hu JavaScript assets loaded. And I can see the same list flattening problem with JSON.stringify. When I commented out

<script src="http://sportgeza.hu/assets/js/prototype.min.js"></script>

The problem goes away... which led me to this StackOverflow discussion

[edsu]

I opened mozilla/jschannel#17 to see if the fix outlined at StackOverflow would make sense to have in jschannel, or if we should use it in h.

[csillag]

Nice work tracking this down!
Now the question is, how do we solve this without breaking the pages
actually depending on the problematic part of Prototype.

[edsu]

I think the Stack Overflow discussion outlines a reasonable fix. I think it would be useful to add the fix to jschannel, since it relies so heavily on JSON.stringify working properly. I added an issue for it, and will send them a pull request shortly.

[edsu]

I've got a pull request open to jschannel folks that fixes this problem. I tested by copying the jschannel.js into place into h, and the problem with http://sportgeza.hu/sport/vivovb/2013/08/10/iszonyu_fajdalmas_vereseg_a_romanoktol/ (and any other page that happens to use a broken prototype.js) goes away.

[csillag]

Great! (I have commented at your commit, here: edsu/jschannel@ef3dc3d )

[edsu]

I see your comment and raise you another comment (and commit).

[csillag]

Thank you. Now I like it 100%.

[csillag]

I think you should just update the jschannel copy we are using with your fix, and the close this bug. (I see no reason to wait for jschannel to accept your PR.)

[edsu]

If jschannel is updated and we pull in that update, and they haven't merged the pull request, then we quietly get this bug again.

[csillag]

True. We should be careful when we update jsChannel. (By the way, usually we don't update our dependencies, unless there is a serious reason to do so.)

Do you have any idea how could we avoid this silent regression? (Except of course postponing merging your fix until upstream merges it, too.)

[tilgovi]

We could add this to our own code, instead of jschannel.