Help needed with bug in Javascript file please!

[Cranial Fool]

Hi all,

A developer has provided me with some scripts that display clusters of markers and info windows on a Google Map (v3 API). The markers side of things works well but there is a problem with the info windows. Unfortunately I didn't notice until after I paid him and I've been trying without success to get him to fix it for six weeks now.

I've narrowed the problem down to a javascript file. It's 654 lines long so I won't paste it all in here, but if anyone wants me to put up any other sections please ask.

The info windows appear at a pre-determined interval, fading in and out using jquery I believe. It is supposed to be a random sample of 30 that loads when the map loads, and then they should just cycle through the list one by one. The script triggers a php file to run, and this fetches the data from a MySQL table.

It all works - I can see in Firebug that the php file requests the marker clusters and gets the data back fine. The problem is that it asks for the sample of 30 for the info windows thousands of times. I can see that the same sample of 30 is always returned. If I refresh the page, a different sample of 30 gets hammered. What usually happens is that the firewall on my server cuts this process off. There are also tons of aborted requests, which surely cannot be right.

I've tried lots of things to fix it, but I am pretty much a novice when it comes to javascript.

fetch: function(zoom){
		if(!this.gmap) return ;

		this.clearPopup();
		this.fetchMarkersInfo();
		this.clearOverlays();
		var category = this.selectedCategories();

		var params = 'q=cluster&'+ this.fetchBounds2Param(this.fetchBound())+'&zoom='+this.gmap.getZoom()+(this.gmap.getZoom() == this.gmap.maxZoom? '&markers=1' : '')+(category == '' ? '' : '&cat=' +escape(category));
		
		var obj = this;
		if(this.ajaxConn){
			this.ajaxConn.abort();
			this.ajaxConn = null;
		}
		this.enableGMap(false);	
		this.ajaxConn = $.ajax({url: URL_API,
								data: params,
								type: 'POST', 
								dataType: 'json',				  
								success: function(json){
									obj.showResults(json);
								},	
								complete: function(jqXHR, textStatus){						
									obj.enableGMap(true);
									if(obj.popupActive) return ;
									obj.popupActive = true;
									if(obj.popupTimeout)
										clearTimeout(obj.popupTimeout);
									obj.popupTimeout = setTimeout('Clusters.popupNext();', POPUP_DELAY);
								}
							   });		
	},

	fetchMarkersInfo: function(){
		if(!this.gmap) return ;
		if(this.markerInfoCompleted) return ;
		var lastId = (this.markersInfo.length > 0 ? this.markersInfo[this.markersInfo.length-1].id+1 : 1);
		var category = this.selectedCategories();
		
		var obj = this;
		var params = 'q=list&from='+ lastId+ '&'+ this.fetchBounds2Param(this.fetchBound())+(category == '' ? '' : '&cat=' +escape(category))+'&lmt=30';
		
		if(this.ajaxConnPopup){
			this.ajaxConnPopup.abort();
			this.ajaxConnPopup = null;
		}

		this.ajaxConnPopup = $.ajax({url: URL_API,
								data: params,
								type: 'POST', 
								dataType: 'json',				  
								success: function(json){
									obj.addPopupMarkers(json);
									if(!obj.markersInfoCompleted)
									obj.fetchMarkersInfo();
								}
							   });		
	},

I have wondered about those last two lines – if I disable them, the php file only asks for the list once. But then the info windows don't get displayed (although I can still click on a marker and get one that way), and there are still several aborted requests.

Any help would be much appreciated!

[Airshow]

Cranial,

Analysis
It looks as if fetchMarkersInfo is designed to repeatedly fetch 30 markers until a flag is raised to say "enough!".

2 points:

1. The flag is not raised in the code snippet you have posted, but probalby in addPopupMarkers() (or something called by that function).
2. In fetchMarkersInfo, the flag is supposed to be tested twice, but it is coded once as .markerInfoCompleted and once as .markersInfoCompleted (at lines 37 and 55 of your snippet respectively).

The Fix
Search the rest of the code for "markerInfoCompleted" and "markersInfoCompleted" to discover whether the flag is ever raised (ie. set to true) and which spelling is correct.

• If the flag is raised, then correct the spelling at line 37 or 55.
• If the flag is never raised, then comment out lines 55 and 56, and replace with obj.markerInfoCompleted = true; .

Airshow

[Cranial Fool]

Hi Airshow,

Thank you so much for your reply.

.markerInfoCompleted only appears that one time, so I have changed it to .markersInfoCompleted. I can't detect any difference in behaviour now this has been corrected though.

.markersInfoCompleted = true occurs in the very next function in the script:

addPopupMarkers: function(json){
		if(!json) return ;
		if(!json.length && json.length != 0) return ;
		if(json.length<1){
			this.markersInfoCompleted = true;
		}
		else {		
			this.markersInfo = this.markersInfo.concat(json);
		}	

	},

I tried replacing lines 55 and 56 with obj.markerInfoCompleted = true; - this stopped the info windows from working but the repeated fetching continued.

Best regards,
Cranial

[Airshow]

I don't pretend to understand addPopupMarkers.

A jQuery JSON response is an object not an array, but the function addPopupMarkers:

1. tests json.length
2. includes the line this.markersInfo = this.markersInfo.concat(json);

Both imply that the function expects json to be an array.

As the function only raises the flag this.markersInfoCompleted if json.length<1 , I would guess it is intended that fetching of markers should cease only when the server gives a null response. However, if I'm right about .length, the flag will never be raised regardless of what the server returns.

First we need to make sure about json.length . Could you try adding alert(json.length); immediately before obj.addPopupMarkers(json); . Let me know what it gives (and be prepared for it to fire again and again - you may need to crash the browser to get out of it, though timely use of the browser's back button may do it).

Airshow

[Cranial Fool]

Hi Airshow,

Thanks again for coming back.

The alert says 30 each time.

I was after a function that fetched a random sample of 30 for the info windows each time the map is loaded, so with my limited knowledge I would not ever expect a scenario where the server returned a null response. Having said that I have just run it in Firebug again and this time different batches of 30 were fetched each time. The last time I ran it, it kept fetching the same batch. Not sure how that has changed. Perhaps it is fetching the entire table in batches of 30. I guess that would explain the use of the flag.

Best regards,
Cranial

[Airshow]

OK, so json.length exists and is always 30. Not what I was expecting but let's assume that json, for whatever reason, is an array.

First try replacing the ajax block in fetchMarkersInfo with,

this.ajaxConnPopup = $.ajax({url: URL_API,
			data: params,
			type: "POST",
			dataType: 'json',
			success: function(json){
				if(!json || !json.length) { return; }
				obj.markersInfo = obj.markersInfo.concat(json);
				obj.markersInfoCompleted = true;
			}
		});

This should fetch your 30 random markers and unconditionally raise the flag so they won't be fetched again. This may or may not work. It should at least prevent repeated fetching of markers.

****

My only concern is that back in fetch, the bulk of the code may (I don't know for sure for sure) be dependent on the markers having been fetched, which will not be the case, at least the first time that fetch is called; reason being that the ajax is asynchronous - in other words fetchMarkersInfo will not be truly complete when fetch gets to the line this.clearOverlays(); .

If this is the case, then everything in fetch from this.clearOverlays(); onward needs to be made into a callback function for execution at the end of fetchMarkersInfo's ajax success fn. (It's possible that the original programmer didn't know this technique and needed to jump through several awkward hoops as a workaround).

Follow me? Probably not unless you are familiar with asychronicity and callbacks.

Anyway, the upshot would be a refactoring of fetch and fetchMarkersInfo as follows:

fetch: function(zoom){
		if(!this.gmap) return ;

		var obj = this;
		this.clearPopup();

		var callbackFn = function(){//define a callback for execution when fetchMarkersInfo has completed.
			this.clearOverlays();
			var category = this.selectedCategories();

			var params = 'q=cluster&'+ this.fetchBounds2Param(this.fetchBound())+'&zoom='+this.gmap.getZoom()+(this.gmap.getZoom() == this.gmap.maxZoom? '&markers=1' : '')+(category == '' ? '' : '&cat=' +escape(category));

			if(this.ajaxConn){
				this.ajaxConn.abort();
				this.ajaxConn = null;
			}
			this.enableGMap(false);
			this.ajaxConn = $.ajax({
				url: URL_API,
				data: params,
				type: "POST",
				dataType: 'json',
				success: function(json){
					obj.showResults(json);
				},
				complete: function(jqXHR, textStatus){
					obj.enableGMap(true);
					if(obj.popupActive) { return; }
					obj.popupActive = true;
					if(obj.popupTimeout) { clearTimeout(obj.popupTimeout); }
					obj.popupTimeout = setTimeout(Clusters.popupNext, POPUP_DELAY);
				}
			});
		};
		this.fetchMarkersInfo(callbackFn);//callbackFn will be executed when fetchMarkersInfo's ajax has completed.
	},

	fetchMarkersInfo: function(callbackFn){
		callbackFn = (!callbackFn) ? function(){} : callbackFn;//in case fetchMarkersInfo is called without a callbackFn
		if(!this.gmap) return ;
		if(this.markersInfoCompleted){ callbackFn(); }//in case fetch fires again after markersIno has been fetched
		else{
			var lastId = (this.markersInfo.length > 0 ? this.markersInfo[this.markersInfo.length-1].id+1 : 1);
			var category = this.selectedCategories();
			
			var obj = this;
			var params = 'q=list&from='+ lastId+ '&'+ this.fetchBounds2Param(this.fetchBound())+(category == '' ? '' : '&cat=' +escape(category))+'&lmt=30';
			
			if(this.ajaxConnPopup){
				this.ajaxConnPopup.abort();
				this.ajaxConnPopup = null;
			}

			this.ajaxConnPopup = $.ajax({url: URL_API,
				data: params,
				type: "POST",
				dataType: 'json',
				success: function(json){
					if(!json || !json.length) { return; }
					obj.markersInfo = obj.markersInfo.concat(json);
					obj.markersInfoCompleted = true;
					callbackFn();
				}
			});
		}
	},

You will appreciate, I'm working half blind here as I can't actually run the code and my suggestions may need debugging.

Airshow

[Cranial Fool]

Hi,

When I replaced the ajax block in fetchMarkersInfo with your new code, the application first fetched 30 marker infos, then there were 5 aborted requests, then it fetched the clusters twice. I ran this several times and once the info windows worked! But since then I have not been able to replicate that.

After I replaced fetch and fetchMarkersInfo with your new code, 4 requests were made to the server, of which 2 were aborted and 2 were requests for 30 markers info. There were no requests for clusters and nothing was displayed on the map.

Many thanks for your ongoing help!

Best regards,
Cranial

[Cranial Fool]

In case it helps, the full script is here:

http://83.223.104.166/clusters.js

and one without any changes except for the function name corrected:

http://83.223.104.166/clusters_original.js

Best regards,
Cranial

[Cranial Fool]

I've done some more fiddling, and I've discovered that your first solution (replacing the ajax block in fetchMarkersInfo) gets the info windows working every time with a table that only has a couple of hundred rows.

Removing the line this.fetchMarkersInfo(); in the fetch function disables the info windows without affecting the clusters. There's a good chance that the info window functionality was bolted on to a pre-existing clustering script.

Best regards,
Cranial

[Airshow]

Hi Cranial,

Aha, that's interesting. Looking at the code in clusters.js, there certainly appears to be two programming styles. In particular, there are many opportunities for using jQuery but it is only lightly used in the methods fetch, fetchMarkersInfo, popupNext and showResults.

Could you explain in broad terms more about the relationship between "markers" and "clusters" and how the 30-limit applies please. I can discern only so much from the code and it is clearly germane to the bug.

The code also makes provision for things called "categories" but there's evidence they are not used in your application.

Airshow

[Cranial Fool]

Hi Airshow,

Markers start life as rows in the table containing latitude, longitude, category and data for the info windows.

“Clusters” refers to geographically-clustered markers. Ajax.php (defined as URL_API in clusters.js) does the clustering work and sends just the cluster data back to the browser, along with any individual markers that aren't part of a cluster. The info window data for those individual markers is also fetched with the clustering function, so that they can be clicked on.

A different part of the same php script provides the sample of markers used for the automatically cycling info windows (“popups”). The limit of 30 was intended to minimise the amount of data sent to the browser – just enough for a cool display without tying up too much bandwidth.

I can't think why there needs to be any inter-dependency between the clustering part of the script and the popups, apart from the convenience of being able to call fetch which in turn calls fetchMarkersInfo. In fact there is an adjustable time delay before the popup cycle starts, because it looks wrong if it begins before the markers have appeared. So if there was any inter-dependency, it would be better if fetchMarkersInfo was dependent on fetch having completed.

Each marker has a category, and users can select the categories they want displayed.

Thanks for sticking with me! :-)

Best regards,
Cranial

[Airshow]

Thanks Cranial, that adds a lot of meat to the skeleton.

I'm tied up for a short while, maybe a day or so, but will have another look at the code when I'm free.

Airshow

[Cranial Fool]

Ok thank you!

Cranial

[Airshow]

Cranial,

I've not forgotten you. Still trying to work out exactly how all the code works.

Currently exploring a certain train of thought.

Oh yes, I passed your js file through jslint, which discovered a probable coding error at line 540:

//-Find
else if(nonNull(bound.lat1))
//-Replace with
else if(nonNull(b.cluster.lat1))

Maybe you can rerun your last set of tests with the fix in place.

It may help but it's possibly in a block of code that doesn't get executed.

More later.

Airshow

[Cranial Fool]

Hi Airshow,

Great to hear from you again. Thanks for spotting the error.

I've replaced the code and re-run all the same tests – everything seems the same as before, although it must have helped.

The first version of the script that the developer gave me didn't exhibit this problem – only two requests to the server – one for the clusters and one for the sample of 30. No repeats and no aborted requests. I've been reluctant to go back to it because it had several bugs that were ironed out. But in the process this problem was introduced. Anyway in case it helps I've put it up at http://83.223.104.166/clusters_old.js – please feel free to ignore it if not!

Best regards,
Cranial

[Airshow]

OK, hopefully the earlier version of the script will be enlightening. I'll have a look now.

Do you recall in any detail what bugs needed to be ironed out from that version?

Airshow

[Cranial Fool]

Hi Airshow,

I have a complete history of all the versions and all the correspondence, so it's no problem to pick through it. The server-thrashing bug appeared early on (I now realise!) when these issues were worked on:

1. Popup properties added (see top of script) including delay before first popup appears

2. Restricting popups to zoom levels 2-4

3. Making popup sample random instead of first 30

4. Popup sequence now continuous instead of stopping at the end of the sample

5. Adding CSS style for all info windows

6. Making all info windows fade in and out

7. Attempting to fix erratic cycling of popups

8. Linked to http://83.223.104.166/infobubble.js for styling purposes - not managed to find out how it's linked! :-(

I've put up the version that first exhibited the problem at http://83.223.104.166/clusters_mk3.js

2, 5, 6 and 7 were not fully resolved in the Mk3 version and were worked on again later. Also later, the number range for each cluster was changed.

Please let me know if you need any more info.

Best regards,
Cranial

[Airshow]

Cranial,

Not a solution I'm afraid but hopefully a way ahead.

After a lot of code trawling, I think I know how it is supposed to work - at least, I know what has been programmed.

Concentrating on markers not clusters ......

Markers are fetched from the server in batches of 30 until they are all served. At that point a flag is raised to prevent further fetch attempts. Thus, the pool of fetched markers starts small and grows as more and more markers are fetched.

Markers shown in the popup (one at a time) are chosen at random from the available pool.

The fetching appears to be done in batches to allow the choice/display of markers to begin before all markers have been served (which might take a while).

Changes to map bounds (ie. on zooming and panning) cause the whole process to be restarted, with different search parameters sent to the server. When this happens, an abort mechanism ensures that any in-progress fetching of markers is interrupted before the marker pool is cleared and a new fetching cycle begins (and similarly for clusters).

I guess that changes to the category selection will also restart the fetch cycle, but the code to stimulate that must be in a different file.

As far as I can tell, no aborts should occur except in response to user interactions after the map has initialised.

It is possible that some of this (particularly the randomisation?) is not exactly what you specified, and/or your programmer did not envisage the live data sets to be quite as large as they are. However, this does not account for the same batch of 30 markers being repeatedly requested as you say happens under some conditions. That aspect has me stumped.

I think your way ahead is to respicify or reassert your original specification in terms similar to my process description above. Then, the original programmer (or me if I can find the time) will have something to work to.

Server-thrashing arises wholly from 3. Making popup sample random instead of first 30 . This part of the specification needs to be addressed if server-thrashing is to be avoided.

I hope this helps.

Airshow

[Cranial Fool]

Hi Airshow,

Thank you so much for such a comprehensive analysis.

I've tried to PM you but I got the response "Airshow has exceeded their stored private messages quota and cannot accept further messages until they clear some space."

Best regards,
Cranial

[Airshow]

Cranial,

I have cleared some space. Try again now.

Airshow