Bug? ajax requests fail in firefox ( setRequestHeaders )

[Jimbo]

Hi guys..

I've been trying to chase down a problem in Firefox.. basically ajax
requests aren't working.

As far as I can tell this problem is in 1.5.0 ( I've just downloaded a
fresh copy and checked.. )

The problem is that for some reason setRequestHeaders ( part of the
ajax request routines ) tries to add the header "toJSONString" which
is a function!! ... along with "X-Request-With", "X-Prototype-Version"
and "Accept"..

... now, I can't find how/why this is getting included into the
headers object so in my haste I've just patched it up. I'm also
frankly not sure whether this is a bug everyone else sees or I'm just
going a little loopy!. ..

Anyhow.. my patch is this: in function setRequestHeaders ( about line
900 in pt.1.5.0 ), the last line reads:

this.transport.setRequestHeader(name, headers[name]);

... simply prepend the line with:

( typeof headers[name] != 'function' ) &&

... to read:

( typeof headers[name] != 'function' ) &&
this.transport.setRequestHeader(name, headers[name]);

... and voila.. :-) ... a hack I know, but it seems to work

... I've also done a diff against ajax.js if thats of more use to
someone:

http://logan.mediaisotope.com/patches/prototype_ajax_bug_setRequestHeaders.patch

... cheerio.
Jimbo

[Christophe Porteneuve]

Hey Jimbo,

This is not a Prototype issue. Besides Prototype, you're using a
JSON-related library that patches Object.prototype to provide it with a
toJSONString method, probably http://www.json.org/json.js. This issue
is widely held against its official implementation...

This breaks just about every for...in loop in every piece of JS code
you'll ever run when this lib is loaded (including the one in
setRequestHeaders). Which is why extending Object.prototype is widely
regarded as a malpractice. Actually, earlier versions of Prototype used
to do this, and quickly reverted to a cleaner behavior.

Note that Prototype's trunk (current development version) finally adds
JSON-related methods, so in the next point release you'll have them
without the hassle. However, we use a namespaced Object.toJSONString
method for generic objects (and a regular method for specific object
types), to avoid this problem.

--
Christophe Porteneuve aka TDD
t...@tddsworld.com

[Jimbo]

Hi Christophe,

100% guilty as charged! ;-) ... we're using both json.js &
prototype.js and I guess I never thought to look elsewhere because
it's something that changed when we moved from 1.4.0 to 1.5.0.

This seems a bit of a fundamental problem with json.js doesn't it?

I see what you mean though - and it extends Array and String in this
way too ... is there a good json serializer alternative that you know
of or should I wait for the pt implementation

Thanks anyhow,

ATB,
Jim

On Feb 6, 11:34 am, Christophe Porteneuve <t...@tddsworld.com> wrote:
> Hey Jimbo,
>
> This is not a Prototype issue. Besides Prototype, you're using a
> JSON-related library that patches Object.prototype to provide it with a

> toJSONString method, probablyhttp://www.json.org/json.js. This issue

[all...@gmail.com]

For the benefit of anyone else with this issue...

I ran into this when using rico 1.1.2 with prototype 1.5.0. Same
issue. I dropped rico off the page and Ajax.Request started working
again.

[Yanick]

This is what I did to solve the problem

setRequestHeaders: function() {
....

// modified foreach iteration
$H(headers).each( function(header) {
this.transport.setRequestHeader(header[0], header[1]);
}.bind(this) );

// for (var name in headers)
// this.transport.setRequestHeader(name, headers[name]);
}

The problem encountered was that the foreach was trying to add the
function 'extend' to the headers, and was causing it to fail with an
exception in the XMLHttpRequest object under FF :

"Component returned failure code: 0x80080057 (NS_ERROR_ILLEGAL_VALUE)
[nsIXMLHttpRequest.setRequestHeader]" (blah blah blah) location: "JS
frame :: http://.../prototype.js :: anonymous :: line 921" data: no"

I didn't do much extensive tests, but with the .each() function hooked
to a hash map of the header object seems to work nicely. ....And since
there's a .each() function, why not use it ? Just a thought.

-Yanick

On 9 fév, 15:20, "alle...@gmail.com" <alle...@gmail.com> wrote:
> For the benefit of anyone else with this issue...
>
> I ran into this when using rico 1.1.2 with prototype 1.5.0. Same

> issue. I dropped rico off the page and Ajax.Requeststarted working

[Colin Mollenhour]

While I personally wouldn't use any library or code that broke "for... in" loops, this is the real world and I think making the change you suggested is a good idea. It would certainly help reduce the number of people coming to the discussion board over and over for this same reason even though it technically isn't Prototype's fault. Since this is not a performance-critical piece of code, I see no reason not to implement your suggestion.  Submit a patch as described here ( http://www.prototypejs.org/contribute ) and provide your best explanation for it in the patch description and hope everyone will agree... :)

I'll be honest and point out that the Event patch I submitted uses for...in, but I think it is justified since Event is a *very* performance critical class and the overhead of $H and .each may not be worth it. I haven't tested it, but I imagine a 1000+ element loop being called 1000+ times would make a difference, in this case it won't make a difference.

Colin

[Yanick]

I agree. I will submit the patch. Thank for your specification on the
overhead matter, haven't really thought of that (but as you said,
performance was not a concern in this function, and I would assume
that there won't be 1000's elements in the header object !)

Cheers.

-Yanick

On 14 fév, 14:53, Colin Mollenhour <eliteii...@mollenhour.com> wrote:
> While I personally wouldn't use any library or code that broke "for... in" loops, this is the real world and I think making the change you suggested is a good idea. It would certainly help reduce the number of people coming to the discussion board over and over for this same reason even though it technically isn't Prototype's fault. Since this is not a performance-critical piece of code, I see no reason not to implement your suggestion. Submit a patch as described here (http://www.prototypejs.org/contribute) and provide your best explanation for it in the patch description and hope everyone will agree... :)

> I'll be honest and point out that the Event patch I submitted uses for...in, but I think it is justified since Event is a *very* performance critical class and the overhead of $H and .each may not be worth it. I haven't tested it, but I imagine a 1000+ element loop being called 1000+ times would make a difference, in this case it won't make a difference.
> Colin

> Yanick wrote:This is what I did to solve the problem setRequestHeaders: function() { .... // modified foreach iteration $H(headers).each( function(header) { this.transport.setRequestHeader(header[0], header[1]); }.bind(this) ); // for (var name in headers) // this.transport.setRequestHeader(name, headers[name]); } The problem encountered was that the foreach was trying to add the function 'extend' to the headers, and was causing it to fail with an exception in the XMLHttpRequest object under FF : "Component returned failure code: 0x80080057 (NS_ERROR_ILLEGAL_VALUE) [nsIXMLHttpRequest.setRequestHeader]" (blah blah blah) location: "JS frame ::http://.../prototype.js:: anonymous :: line 921" data: no" I didn't do much extensive tests, but with the .each() function hooked to a hash map of the header object seems to work nicely. ....And since there's a .each() function, why not use it ? Just a thought. -Yanick On 9 fév, 15:20,"alle...@gmail.com"<alle...@gmail.com>wrote:For the benefit of anyone else with this issue... I ran into this when using rico 1.1.2 with prototype 1.5.0. Same issue. I dropped rico off the page and Ajax.Requeststarted working again. On Feb 6, 6:58 am, "Jimbo"<jimbomorri...@gmail.com>wrote:Hi Christophe,100% guilty as charged! ;-) ... we're using both json.js & prototype.js and I guess I never thought to look elsewhere because it's something that changed when we moved from 1.4.0 to 1.5.0.This seems a bit of a fundamental problem with json.js doesn't it?I see what you mean though - and it extends Array and String in this way too ... is there a good json serializer alternative that you know of or should I wait for the pt implementationThanks anyhow,ATB, JimOn Feb 6, 11:34 am, Christophe Porteneuve<t...@tddsworld.com>wrote:Hey Jimbo,This is not a Prototype issue. Besides Prototype, you're using a JSON-related library that patches Object.prototype to provide it with a toJSONString method, probablyhttp://www.json.org/json.js. This issue is widely held against its official implementation...This breaks just about every for...in loop in every piece of JS code you'll ever run when this lib is loaded (including the one in setRequestHeaders). Which is why extending Object.prototype is widely regarded as a malpractice. Actually, earlier versions of Prototype used to do this, and quickly reverted to a cleaner behavior.Note that Prototype's trunk (current development version) finally adds JSON-related methods, so in the next point release you'll have them without the hassle. However, we use a namespaced Object.toJSONString method for generic objects (and a regular method for specific object types), to avoid this problem.-- Christophe Porteneuve aka TDDt...@tddsworld.com

[Jimbo]

If speed is an issue is it not more lightweight to simply typeof each
item you're adding to the headers rather than use $H() ... a la:

http://logan.mediaisotope.com/patches/prototype_ajax_bug_setRequestHeaders.patch

... or am I missing the point?

J.

> > Yanick wrote:This is what I did to solve the problem setRequestHeaders: function() { .... // modified foreach iteration $H(headers).each( function(header) { this.transport.setRequestHeader(header[0], header[1]); }.bind(this) ); // for (var name in headers) // this.transport.setRequestHeader(name, headers[name]); } The problem encountered was that the foreach was trying to add the function 'extend' to the headers, and was causing it to fail with an exception in the XMLHttpRequest object under FF : "Component returned failure code: 0x80080057 (NS_ERROR_ILLEGAL_VALUE) [nsIXMLHttpRequest.setRequestHeader]" (blah blah blah) location: "JS frame ::http://.../prototype.js::anonymous :: line 921" data: no" I didn't do much extensive tests, but with the .each() function hooked to a hash map of the header object seems to work nicely. ....And since there's a .each() function, why not use it ? Just a thought. -Yanick On 9 fév, 15:20,"alle...@gmail.com"<alle...@gmail.com>wrote:For the benefit of anyone else with this issue... I ran into this when using rico 1.1.2 with prototype 1.5.0. Same issue. I dropped rico off the page and Ajax.Requeststarted working again. On Feb 6, 6:58 am, "Jimbo"<jimbomorri...@gmail.com>wrote:Hi Christophe,100% guilty as charged! ;-) ... we're using both json.js & prototype.js and I guess I never thought to look elsewhere because it's something that changed when we moved from 1.4.0 to 1.5.0.This seems a bit of a fundamental problem with json.js doesn't it?I see what you mean though - and it extends Array and String in this way too ... is there a good json serializer alternative that you know of or should I wait for the pt implementationThanks anyhow,ATB, JimOn Feb 6, 11:34 am, Christophe Porteneuve<t...@tddsworld.com>wrote:Hey Jimbo,This is not a Prototype issue. Besides Prototype, you're using a JSON-related library that patches Object.prototype to provide it with a toJSONString method, probablyhttp://www.json.org/json.js. This issue is widely held against its official implementation...This breaks just about every for...in loop in every piece of JS code you'll ever run when this lib is loaded (including the one in setRequestHeaders). Which is why extending Object.prototype is widely regarded as a malpractice. Actually, earlier versions of Prototype used to do this, and quickly reverted to a cleaner behavior.Note that Prototype's trunk (current development version) finally adds JSON-related methods, so in the next point release you'll have them without the hassle. However, we use a namespaced Object.toJSONString method for generic objects (and a regular method for specific object types), to avoid this problem.-- Christophe Porteneuve aka TDDt...@tddsworld.com- Hide quoted text -
>
> - Show quoted text -

[Yanick]

Actually, look at this ticket, I've found a yet better implementation.

http://dev.rubyonrails.org/ticket/7566

...waiting for replies from the Prototype team.

-Yanick

On 15 fév, 07:48, "Jimbo" <jimbomorri...@gmail.com> wrote:
> If speed is an issue is it not more lightweight to simply typeof each
> item you're adding to the headers rather than use $H() ... a la:
>

> http://logan.mediaisotope.com/patches/prototype_ajax_bug_setRequestHe...

[Colin Mollenhour]

The point I was trying to make is that in this case, the overhead in calling $H() is completely moot since you'll never have more than a handful of requests running and never with more than a few headers. Since $H() is safer and this particular usage of it is not performance critical, then we might as well use it. Now if you are running a loop of 1000 elements 1000 times that might be a different story. Honestly I don't know what the performance difference is but I'd imagine it can add up.. You  are checking if each key exists in Hash.prototype (32 of them always will, that's 32 wasted iterations), creating an array of two elements, setting hash keys for the same two elements, then passing those to your iterator function.  Don't get me wrong, I love the Hash and Enumerable classes, and I use them all the time, just not for really big tasks.

Colin