Fix null Tile#buffers error

[lucaswoj]

fixes #1983

I think this error is triggered every time setData is called on a GeoJSON source used by a line error.

I was able to replicate the error by instantiating mapbox-gl-draw on the latest mapbox-gl-js.

[lucaswoj]

cc @ansis @kelvinabrokwa

[lucaswoj]

As a replacement for hasLayerData, getElementGroups should ensure that Tile#buffers is populated b24b6a1#diff-5de49be07285027d44a5f4cd57d7e395L146

[dylan]

How long before this usually deployed? Also, is there a nice way to grab these latest changes in case the roll out is slow?

[jfirebaugh]

Test?

[dylan]

Sorry, I got a local copy running. Trying it out now. :)

[lucaswoj]

@jfirebaugh I'm still scratching my head on coming up with a good regression test here.

• we don't have any unit tests for Tile
• this bug relies on interplay between Workers and the main thread, which we usually test in test-suite, but we can't run a setData call in test-suite

[jfirebaugh]

That's a good answer. :) I just wanted to make sure it was on your mind. If this was caused by the fix to #1933, then we had two serious regressions in as many releases, and they were related. That to me is an indication that we should invest some time into figuring out how to get this code under test.

[mourner]

Object.keys({}) is still truthy. You'll need to check for length probably.

[lucaswoj]

Thank you. Pushing a fix shortly.

Side note: the !Object.keys(this.buffers) branch is total cargo cult programming. It is never triggered AFIK but I'm preserving it as a refactor of hasLayerData. Maybe that's a mistake and it should be removed altogether.

[lucaswoj]

I understand the problem a little better now. Tile#buffers is null because of https://github.com/mapbox/mapbox-gl-js/blob/null-buffers-1983/js/source/tile.js#L57 and https://github.com/mapbox/mapbox-gl-js/blob/null-buffers-1983/js/source/tile.js#L109. I'm going to remove !Object.keys(this.buffers).length entirely.

[lucaswoj]

we had two serious regressions in as many releases, and they were related. That to me is an indication that we should invest some time into figuring out how to get this code under test.

Agreed. This has been really bad. I think the long term solution should include

1. thorough integration testing of GeoJSON sources in mapbox-gl-js (might jive with Add "skipWorker" option to GeoJSON sources [DO NOT MERGE] #1979)
2. refactoring to make the codepaths used by GeoJSON more similar to those used by vector tile sources (namely stuff like this)

[lucaswoj]

@jfirebaugh @mourner after a little more digging, I found a better solution to the problem, 👀?

fixes #1991 now

(unfortunately this approach doesn't make the PR any more testable)

[lucaswoj]

merged in 70f0677