1409726 - Could vacuum be upsetting places (at least during tests)?

Status: NEW

(Toolkit :: Storage, enhancement, P3)

Description

[Mark Banner (:standard8)]

We've been getting strange intermittent errors in the places mochitests. They are typically happening like this:

<test start>
...
[Parent 2341, Main Thread] WARNING: Unable to vacuum database: places.sqlite - 1 cannot VACUUM from within a transaction: file /builds/worker/workspace/build/src/storage/VacuumManager.cpp, line 246
[Parent 2341, Main Thread] WARNING: Places.sqlite vacuum failed.: 'aSucceeded', file /builds/worker/workspace/build/src/toolkit/components/places/nsNavHistory.cpp, line 2883
...
TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_controller_onDrop.js | uncaught exception - TypeError: aUrlString is null at PUIU_guessUrlSchemeForUI@resource:///modules/PlacesUIUtils.jsm:1123:5
<and lots more similar errors>

e.g. 

https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=133954321&lineNumber=8825
https://treeherder.mozilla.org/logviewer.html#?job_id=137192734&repo=mozilla-central&lineNumber=7255

Although idle-daily that causes vacuum should in theory be disabled for mochitests, for some reason I'm not quite sure that's true currently.

In any case, the fact that we keep seeing this across different tests is concerning. The vacuum message could be a red-herring, but it does seem quite suspicious to me.

Comment 1

[Marco Bonardo [:mak]]

it's possible vacuum causes some synchronous statements to bail out, yes. The reported case here is just vacuum manager giving up because it can't run in the middle of an existing transaction, this should not cause troubles.

Unfortunately for the first case, the only thing we can do is avoid consumers mixing up sync and async apis, Places is moving towards that but we're not there yet.