“Gah: trying to removing cached tile even though it's still in the DOM” message #39

Closed
kkaefer opened this Issue Jun 21, 2011 · 32 comments

Projects

None yet

6 participants

@kkaefer
kkaefer commented Jun 21, 2011
// I'm leaving this uncommented for now but you should never see it:
alert("Gah: trying to removing cached tile even though it's still in the DOM");

Turns out that IE sometimes displays this. Unfortunately, I couldn't figure out how to reproduce this issue.

@RandomEtc
Contributor

Is this in IE8? I've seen that in IE8 as well, but only with multiple maps on the same page. Is there a specific example (or your own sample) that causes the error for you?

My theory is that it's something to do with the "unique" tile IDs not being unique all the time. This was more of a problem when we had a fast Coordinate.toKey() function. Now we're using string concatenation again it's easier to adapt to add the map id as a prefix. This might be the fix, I don't know... more info about which Modest Maps version and which IE version would help narrow it down.

@RandomEtc
Contributor

It might also be an issue with IDs that start with a number, since this is technically not standard (X)HTML, though it's allowed in HTML5. If you're experiencing this issue, please let us know if it is fixed by specifying a different doctype, or by overriding Coordinate.toKey, something like:

com.modestmaps.Coordinate.prototype.toKey =  function() {
    return [ 'c', this.zoom, this.row, this.column ].join(',');
}

This would confirm that it's a numeric key issue. Testing for duplicate keys with multiple maps/layers will require adding a per-layer prefix to the id, which is more complex.

@shawnbot
Collaborator

I've been bumping up against this on the layers branch using IE, and it's killing me. I don't think it's a numeric key issue, because adding the "c" prefix to the keys didn't help. And I've tried the per-layer prefix to no avail.

The only way I've been able to solve it is by doing both of the above, and:

  1. adding a check in checkCache() to bailing if recentTiles.length is zero (so this.recentTiles.pop() doesn't attempt to pop from an empty array):
if (this.recentTiles.length == 0) {
  // console.warn("0 recent tiles; unable to trim the cache any more");
  break;
}
  1. adding a check in adjustVisibleLevel():
if (tile.id in this.recentTilesById) {
  this.recentTilesById[tile.id].lastTouchedTime = now;
}

The danger, of course, is that somewhere in the request/tile cache, something is leaking memory. But at least it's not throwing hundreds of alert()s anymore.

@shawnbot
Collaborator

And here's a diff from this version if that helps.

@feesta
feesta commented Mar 15, 2012

I'm getting this issue on Chrome (19.0.1068.0). I may be doing something stupid but tried to strip it down to the bare essentials: [http://jsfiddle.net/bsbzq/3/]. If I let it load then refresh, when I zoom out, I get the Gah-error. I'm testing in a fresh incognito tab... thoughts?

@shawnbot
Collaborator

Paging @tmcw: did you track down the cause of this before you fixed it? Because I think your fix might be regressing.

@feesta
feesta commented Mar 15, 2012

I'm also having it happen in Safari 5.1.3 but only when the map is very large (full screen on a large monitor or shrink text so lots of tiles are getting loaded). It happens consistently when panning the map.

@tmcw
Collaborator
tmcw commented Mar 15, 2012

I'm going to pour a little more time into finding the cause of this one, but this bug / error message way predates me getting involved in MM, so I'm not even all that sure of whether it has side-effects? Does this throw an exception in IE or another browser?

@feesta
feesta commented Mar 15, 2012

I'm not getting an exception at the time of the alert, but when I interact with the map after dismissing the alert, I get:
Uncaught TypeError: Cannot set property 'lastTouchedTime' of undefined b.Layer.adjustVisibleLevel modestmaps.min.js:14 b.Layer.draw modestmaps.min.js:14 b.Map.draw modestmaps.min.js:14 b.Map.getRedraw._redraw modestmaps.min.js:14

@shawnbot
Collaborator

I'd rope in @RandomEtc if you want some context. Right now it's throwing alert()s but no exceptions in IE, IIRC. (You might want to double-check, though.) My patch to an earlier version for the Oprah project silenced errors in IE (but probably caused memory leaks), and we weren't seeing the issue in modern browsers at that point. But I have a feeling this bug may have existed for a while and is only cropping up now because we're using Modest Maps in much larger browser windows than before. (Jeff and I both have 27" cinema displays.)

@RandomEtc
Contributor

I've had a hard time reproducing this error in the past but I've never seen it in non-IE browsers and I don't get it myself in Safari or Chrome right now (but I have a small screen).

The assumption has always been that the tiles on screen (counted by numTilesOnScreen in checkCache) are the tiles with records at the beginning of recentTiles. So when records are popped off the end of recentTiles and the corresponding tile is removed from this.tiles by id, they shouldn't be in the DOM. Hence the gah message if they are... because I'm not sure why/how they could be :-/

I'd start by checking that recentTiles is correctly sorted and complete: i.e. confirm that the first numTilesOnScreen records in recentTiles are records for tiles that are actually on screen.

And maybe confirm that I didn't add a silly off-by-one error in there that only gets hit when numTilesOnScreen is bigger than maxTileCacheSize?

Sorry I can't be of more help right now...

@tmcw
Collaborator
tmcw commented Mar 16, 2012

This one sucks.

  • not just numeric ids
  • not undefined bits in the array
  • not numtilesonscreen > max
  • not tile appending happening while check cache is running
@RandomEtc
Contributor

Can anyone confirm if parentNode is the current layer or if it's the document fragment we use while loading?

@tmcw
Collaborator
tmcw commented Mar 16, 2012

So far I haven't caught it being anything other than the current layer. I think this bug predates the document-fragment push as well? Right now I'm a little suspicious of this line: https://github.com/stamen/modestmaps-js/blob/master/src/layer.js#L103

Also thought it might be an async bug; tiles being pushed onto the list while checkCache was running. No luck there.

@tmcw
Collaborator
tmcw commented Mar 16, 2012

Thx @javisantana didn't know it could be replicated without even using tiles... current hypothesis is that adjustVisibleLevel is broken, and this doesn't have much to do with max tiles on screen vs the cache

@RandomEtc
Contributor

Testing without img elements could be causing another separate issue, because the checkCache function is still counting img elements to figure out how many tiles to remove.

I don't think L103 is to blame, but you're right that the adjustVisibleLevel loop could be missing something.

@tmcw
Collaborator
tmcw commented Mar 16, 2012
@feesta
feesta commented Mar 21, 2012

Testing old builds, the last stable build was 3149870 (http://jsfiddle.net/bsbzq/7/). It seems that 4a4dd32 (http://jsfiddle.net/bsbzq/11/) re-introduced the 'gah' issue. It is consistently failing within a few seconds of panning on all subsequent builds.

@tmcw
Collaborator
tmcw commented Mar 21, 2012

@feesta great find... it looks like killer line might be here - 4a4dd32#L0L2013 and moving the lasttouchedtime out of else killed things. Though I can't reproduce the problem locally right now - does this build - http://dl.dropbox.com/u/68059/modestmaps.min.js - fix it for you?

@javisantana

the @shawnbot 's code fixes that line but there is still a problem, checkCache function is removing tiles which are still in DOM. Maybe those tiles are in hidden zoom levels but it depends on the tile and map size. isn't it?

Another workaround is to increase the maxTileCacheSize so tiles in DOM are never flushed (but is not a good solution).

@tmcw
Collaborator
tmcw commented Mar 21, 2012

@javisantana can you test current master?

@javisantana

@tmcw is still throwing the alerts and raising exceptions here 18c96c3#L0R2009

for some reason checkCache is removing the tiles before adjustVisibleLevel does

@javisantana

i've made some small changes and it works (it does not print any exception or "gah")

javisantana@49a4993

@feesta
feesta commented Mar 21, 2012

49a49936f9cb7f7d428de2050b151dce5a57111e has fixed it for my testing. Thanks!

@tmcw
Collaborator
tmcw commented Mar 21, 2012

What browsers have you guys been using to replicate this issue? Haven't been able to get it to fire at all today with FF9/Chrome 13/Safari 5, and would definitely like a test case before pulling this into master...

@javisantana

@tmcw

the code i've used to test is https://gist.github.com/2152778 and i've only tested on chrome 17.0.963.79 - OSX.

To replicate it the only thing i did was zoom in and zoom out fast and pan the map at the same time (it puts more tiles on the screen)

@tmcw tmcw added a commit that referenced this issue Mar 21, 2012
@tmcw tmcw More code to alleviate gah pain in #39 00cdb05
@feesta
feesta commented Mar 21, 2012

I've been testing on Chrome 19, Safari 5.1.4, and Firefox 10/11 and it isn't exhibiting the problem it was on the previous build.

@tmcw
Collaborator
tmcw commented Mar 27, 2012

Okay, haven't been able to duplicate the problem; thinking that the fix in 00cdb05 stuck? @shawnbot can you confirm?

@shawnbot
Collaborator

Confirming; please stand by...

@shawnbot
Collaborator

Looks good to me! Thanks for tackling this, Tom; it's a big one.

@tmcw
Collaborator
tmcw commented Jun 13, 2012

Given that the modestmaps/modestmaps-js branch removes the code surrounding this area, and this branch contains the fix, I think we can call this one closed.

@tmcw tmcw closed this Jun 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment