==7466== Invalid read of size 8
==7466== at 0x5F1D0B: mbgl::TransformState::getAngle() const (transform_state.cpp:159)
==7466== by 0x700CD2: mbgl::Source::tileLoadingCompleteCallback(mbgl::TileID const&, mbgl::TransformState const&, bool) (source.cpp:574)
==7466== Address 0xf1d1508 is 56 bytes inside a block of size 120 free'd
==7466== at 0x4C2A4BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7466== by 0x70AD61: std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (mbgl::Source::*)(mbgl::TileID const&, mbgl::TransformState const&, bool)> ()(mbgl::Source*, mbgl::TileID, mbgl::TransformState, bool)> >::_M_destroy(std::_Any_data&, std::integral_constant<bool, false>) (functional:1894)
==7466== by 0x7080EB: std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (mbgl::Source::*)(mbgl::TileID const&, mbgl::TransformState const&, bool)> ()(mbgl::Source*, mbgl::TileID, mbgl::TransformState, bool)> >::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) (functional:1918)
==7466== by 0x535033: _ZNSt8functionIFvvEEaSEDn (functional:2277)
==7466== by 0x56A3F5: mbgl::WorkTask::cancel() (work_task.cpp:34)
==7466== by 0x56146C: mbgl::WorkRequest::~WorkRequest() (work_request.cpp:13)
==7466== by 0x50C5E0: std::default_delete<mbgl::WorkRequest>::operator()(mbgl::WorkRequest*) const (unique_ptr.h:76)
==7466== by 0x50C411: std::unique_ptr<mbgl::WorkRequest, std::default_delete<mbgl::WorkRequest> >::reset(mbgl::WorkRequest*) (unique_ptr.h:344)
==7466== by 0x50C374: std::unique_ptr<mbgl::WorkRequest, std::default_delete<mbgl::WorkRequest> >::operator=(std::unique_ptr<mbgl::WorkRequest, std::default_delete<mbgl::WorkRequest> >&&) (unique_ptr.h:251)
==7466== by 0x743136: mbgl::TileData::reparse(mbgl::Worker&, std::function<void ()()>) (tile_data.cpp:89)
==7466== by 0x6FEA7C: mbgl::Source::handlePartialTile(mbgl::TileID const&, mbgl::Worker&) (source.cpp:250)
==7466== by 0x700338: mbgl::Source::update(mbgl::MapData&, mbgl::TransformState const&, mbgl::Style&, mbgl::GlyphAtlas&, mbgl::GlyphStore&, mbgl::SpriteAtlas&, mbgl::util::ptr<mbgl::Sprite>, mbgl::TexturePool&, bool) (source.cpp:438)
==7466== by 0x68C1A5: mbgl::Style::update(mbgl::MapData&, mbgl::TransformState const&, mbgl::TexturePool&) (style.cpp:82)
==7466== by 0x548F5C: (anonymous namespace)::MockMapContext::update() (resource_loading.cpp:51)
==7466== by 0x548FA3: (anonymous namespace)::MockMapContext::onTileDataChanged() (resource_loading.cpp:56)
==7466== by 0x68CDE4: mbgl::Style::emitTileDataChanged() (style.cpp:212)
==7466== by 0x68CBDB: mbgl::Style::onTileLoaded(bool) (style.cpp:191)
==7466== by 0x700F38: mbgl::Source::emitTileLoaded(bool) (source.cpp:595)
==7466== by 0x700CA2: mbgl::Source::tileLoadingCompleteCallback(mbgl::TileID const&, mbgl::TransformState const&, bool) (source.cpp:573)
The invalid `TransformState::getAngle()` read comes from the following: the
storage for `TransformState` is the [bound parameter in this
callback](https://github.com/mapbox/mapbox-gl-native/blob/5e0775d276f9ea3652bae1
23c4e85cc05ae13bd5/src/mbgl/map/source.cpp#L292-292). The callback itself is
copied into various lambda bindings and eventually winds up as the [after
callback](https://github.com/mapbox/mapbox-gl-native/blob/5e0775d276f9ea3652bae1
23c4e85cc05ae13bd5/src/mbgl/map/tile_data.cpp#L89-89) for the worker task.
On the way out, `tileLoadingCompleteCallback` gets called via this callback
binding. It first does `emitTileLoaded`, which happens to cause [a _new_ work
request to be
queued](https://github.com/mapbox/mapbox-gl-native/blob/5e0775d276f9ea3652bae123
c4e85cc05ae13bd5/src/mbgl/map/source.cpp#L250-250). This cancels the
in-progress work request, [by nulling-out the `after`
callback](https://github.com/mapbox/mapbox-gl-native/blob/5e0775d276f9ea3652bae1
23c4e85cc05ae13bd5/src/mbgl/util/work_task.cpp#L34-34). But in fact, the after
callback is still executing. The next thing it does is [call
transformState.getAngle()](https://github.com/mapbox/mapbox-gl-native/blob/5e077
5d276f9ea3652bae123c4e85cc05ae13bd5/src/mbgl/map/source.cpp#L574), but the
transformState is now gone.
This eliminates a reference loop: VectorTileData holds StyleLayers which
hold StyleBuckets which held Sources which hold VectorTileDatas. This
breaks the StyleBucket -> Source link in favor of manually resolving the
reference where necessary.
Layers are added and removed dynamically on the Map thread when we use shape
annotations and we are iterating style.layers on the Worker thread without
any lock.
Now when we create a [Live|Vector]TileData at the Map thread we give it
a layers list (instead of the Style) and [Live|Vector]TileData will ultimately
make a copy of this list (on the Map thread, so no concurrency with
adding shape annotations).
The copy should be relatively cheap because we are using a shared pointer
for storing the layers on the list.
Use the callback provided for reporting the errors instead of throwing
an exception. The problem with the exception is it will happen on the
Map thread which makes it impossible to the user of our API to catch,
ultimately resulting in a crash.
In the scenario of not providing a callback, we just print an error.
The util::Thread will call the stop() method of the MapContext's RunLoop
which will wait for the pending tasks tied to it to complete.
If we have a request that is timeout'ing, this could mean a really long
wait. Instead, we now send a cleanup message that will reset all the
attributes first (canceling the pending requests) so the the MapContext
gets destructed quickly.
We start loading the resources at the moment we set the style, but we
set the callback for error notification at renderStill(), which happen
afterwards.
If some error occurs in this short window, it was never being notified.
Now we save the last error and we check after we set the callback.
After parsing a tile, it redoes placement for that tile if the angle has
changed since it started parsing. These redo calls were accidentally
done from the worker thread. Assigning the new work to `workRequest`
caused it to cancel itself.
Fix the problem by calling redoPlacement from the map thread.
The tile parsing is done in a worker thread and if it fails,
we set the tile state to "obsolete" and set an error message.
When the completion callback is fired, we check for the error
message and if it is set, we emit the error signal.
The trick here is the signal cannot be emitted directly from
the worker thread because that would violate data ownership rules.
The callback will be called when any of the resources requested
from the network failed to load.
There is not recovery implemented yet, so the closest thing to
reseting the Map object and reloading all the resources would
be setting a new style.
We only emit signals when the number of buckets increase (i.e. the
new data that arrived added a new bucket to the tile).
Also, if we try to reparse a partial tile but it is not possible, we
keep the flag up for trying to reparse it again on the next update,
because the data might have arrived after the bucket that is waiting
for the data was processed but before the end of the parsing.
Fixes#1458
When parsing a tile, we request resources from the network, like
sprites and glyphs.
In the rare case of the resources arriving before the tile is
parsed for the first time, if we don't mark it for being parsed
again, it can end up with a tile eternally stuck in the partial
state.
Partial tiles are now only processed again when there is a new resource
available. If we don't have new resources - like new glyphs - the end result
of the reparsing is the same, because no new SymbolBuckets would are created.
In my tests debugging with PowerTOP, this patch drops the CPU utilization
during the sampling period from 100ms/s to 40ms/s. The improvements on
slow connections (i.e. more time waiting for resources) are even more
noticeable.
We were previously emitting the signal on the thread parsing the
glyph, but signals can only be trigger from the MapThread
because the observers are living there.
For now, keep a pointer so we can subscribe/unsubscribe to events. In
the future, we should move this object completely to ResourceLoader, but
we can't do it now because we delete the ResourceLoader when changing
the style.
When processing a layer, we request the dependencies of all the
SymbolBuckets even if we are not processing it completely.
By doing that we are now requesting glyphs in parallel instead of
sequentially as we were doing before.
This patch is also simplifying the SymbolBucket creation code by getting
rid of the ugly "needsResources" return value passed by reference as
argument.
If the request is cancelled, the response callback won't be called, and
~Source ensures that the request is cancelled. So it's safe to bind
`this` in the lambda.
In order to calculate the collision correctly, the layer ordering
matters. Because of that, when we find the first incomplete symbol
bucket, we stop processing any furter SymbolBucket.
The side effect of this change is, if the first SymbolBucket is the
last to receive its resources, we won't see any label at all on
the screen until the last moment, when everything will be draw at
once.
We keep the Collision object alive longer because we need it
for the SymbolBuckets yet to be created in case more data
resources arrive. When the tile is no longer needed or if it
is completely parsed or obsolete (i.e. in a immutable state)
we throw the Collision object away.
Also add a virtual setState() method to give subclasses a chance
to intercept the change of state. This will be used by VectorTileData
to release resources when the TileData becomes immutable.
