Fix bug in freeing geo polygon memory #104
Conversation
| // Offset of the geofence vertex array pointer within the Geofence struct | ||
| const geofenceArrayOffset = SZ_INT; | ||
| // Free the outer vertex array | ||
| C._free(C.getValue(geoPolygon + geofenceOffset + geofenceArrayOffset, 'i8*')); |
There was a problem hiding this comment.
This was the main issue, including the geofenceArrayOffset when looking up the vertex array pointer.
lib/h3core.js
Outdated
| for (let i = 0; i < numHoles; i++) { | ||
| C._free(C.getValue(geoPolygon + holesOffset + SZ_GEOFENCE * i, 'i8*')); | ||
| C._free(C.getValue(holes + SZ_GEOFENCE * i + geofenceArrayOffset, 'i8*')); |
There was a problem hiding this comment.
I couldn't find a test case that showed the issue, but this was pretty clearly incorrect - we weren't looking up the holes in the right way at all.
Do I need to free holes here too? It seems like I might...
There was a problem hiding this comment.
Updates to free holes
There was a problem hiding this comment.
Yeah, you need to free the holes, they're a pointer in the GeoPolygon struct
| for (let i = 0; i < numHoles; i++) { | ||
| C._free(C.getValue(holes + SZ_GEOFENCE * i + geofenceArrayOffset, 'i8*')); | ||
| } | ||
| C._free(holes); |
There was a problem hiding this comment.
Don't you need to go into each hole struct and free the inner coordinates array, too?
There was a problem hiding this comment.
That's what L257 does - gets the array pointer for each hole and frees it
There was a problem hiding this comment.
Bleh, I clearly shouldn't be reviewing C-memory-management-in-JS code while also working on my own code. Somehow I thought the loop was the outer array being deallocated.
I honestly don't know. The memory starts off clean, so all of the issues are deterministic. The original test case (the pair of polygons in #103) was deterministic, so I could test via script and determine whether the counts matched or not. The test I put into the test suite was deterministic too, but not in the way I'd like - if it mismanaged the memory, that test still passed, but 23 other subsequent tests failed with weird behavior. So I can reasonably assert that I fixed the original bug. But I doubt there's any Valgrind equivalent just for Emscripten-compiled code :/. I'm a bit worried here that I don't have a great way to assert that the |
Closes #103
This fixes some errors in freeing
GeoPolygonmemory, which could get the Emscripten virtual memory management into a bad state.Explanation of the issue: We were expecting the
GeoPolygonreference toGeofenceto be a pointer, but it’s not, the struct is inlined intoGeoPolygon. So I was trying to free the value at the geofence position, because I thought it was a pointer, but I’m actually getting the first value in the Geofence struct, which isnumVerts, and trying to free that. C would probably segfault at this point, Emscripten just gets confused and ends up with undefined behavior in future calls.