D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:75db0004ad02: Replace several Q_ASSERTs with proper 
checks (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12336?vs=33171&id=52870

REVISION DETAIL
  https://phabricator.kde.org/D12336

AFFECTED FILES
  src/engine/documenturldb.cpp

To: bruns, #baloo, #frameworks, ngraham, poboiko, dhaumann
Cc: dhaumann, ngraham, apol, sitter, kde-frameworks-devel, broulik, 
#frameworks, domson, ashaposhnikov, michaelh, astippich, spoorun, bruns, 
abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  I am very much in favour of this change.
  
  In fact, we had this discussion already a long time ago on the mailing list, 
namely that baloo has (had?) zero error handling and simply assuming that 
everything works by adding some Q_ASSERTS.
  
  @bruns You are working on this code for quite a time now. I trust you know 
very much what you are doing much more than possibly anyone else. I know that 
you would like to get a better review, but having a review open for 5+ months 
clearly shows we have an issue elsewhere.
  
  Please proceed and commit - I am taking the liberty to give a +2: If there 
are other issues, the these can still be fixed in followup commits.

REPOSITORY
  R293 Baloo

BRANCH
  db_robustness2

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, #frameworks, ngraham, poboiko, dhaumann
Cc: dhaumann, ngraham, apol, sitter, kde-frameworks-devel, broulik, 
#frameworks, domson, ashaposhnikov, michaelh, astippich, spoorun, bruns, 
abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns edited reviewers, added: ngraham, poboiko; removed: michaelh.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: ngraham, apol, sitter, kde-frameworks-devel, broulik, #frameworks, domson, 
ashaposhnikov, michaelh, astippich, spoorun, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns added a comment.


  In D12336#333853 , @apol wrote:
  
  > If it was silently corrupting the DB maybe the right solution would have 
been to integrate baloo properly with KCrash.
  >  Like @sitter did in https://phabricator.kde.org/D15573.
  
  
  **Silently** - it does not crash, garbage data ends up in the database for 
(mostly) unknown reasons.
  
  Only known reason for zero IDs are races due to renames and similar, where 
e.g. a file is added with some path, and the parent does not exist microseconds 
later (at least not under the previous name).

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: ngraham, apol, sitter, kde-frameworks-devel, broulik, #frameworks, domson, 
ashaposhnikov, michaelh, astippich, spoorun, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in documenturldb.cpp:44
> Shouldn't asserts help to find where the library is being misused?

You gain no information if you crash here. Not the code is wrong, but the 
processed data.

Most importantly, when somebody runs this code, compiled without DEBUG, we will 
run into more and more silent corruption.

As soon as baloo is in a state where it is selfhealing, adding logging becomes 
an option, currently it is not.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: ngraham, apol, sitter, kde-frameworks-devel, broulik, #frameworks, 
ashaposhnikov, michaelh, astippich, spoorun, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> bruns wrote in documenturldb.cpp:44
> This may be accessed quite frequently, so we would get a lot of warnings, 
> spamming the journal, and slowing down the system.
> 
> A backtrace is useless here, as the code processes whatever it finds in the 
> database. The interesting part is how the bad data ended up in the db.
> 
> Doing the checks here avoids spreading any existing corruption.
> 
> The long term solution is to:
> 
> 1. Find the bugs leading to the DB corruption
> 2. Run lowlevel and highlevel sanity checks on existing databases
> 
> Both are on my TODO list, but this will take some time.

Shouldn't asserts help to find where the library is being misused?

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: ngraham, apol, sitter, kde-frameworks-devel, broulik, #frameworks, 
ashaposhnikov, michaelh, astippich, spoorun, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in documenturldb.cpp:44
> Would it make sense to include a warning then? If it's a wrong input, we'll 
> need ways to debug it now that it won't be giving a backtrace.

This may be accessed quite frequently, so we would get a lot of warnings, 
spamming the journal, and slowing down the system.

A backtrace is useless here, as the code processes whatever it finds in the 
database. The interesting part is how the bad data ended up in the db.

Doing the checks here avoids spreading any existing corruption.

The long term solution is to:

1. Find the bugs leading to the DB corruption
2. Run lowlevel and highlevel sanity checks on existing databases

Both are on my TODO list, but this will take some time.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: apol, sitter, kde-frameworks-devel, broulik, #frameworks, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Aleix Pol Gonzalez]

apol added subscribers: sitter, apol.
apol added a comment.


  If it was silently corrupting the DB maybe the right solution would have been 
to integrate baloo properly with KCrash.
  Like @sitter did in https://phabricator.kde.org/D15573.

INLINE COMMENTS

> documenturldb.cpp:44
>  {
> -Q_ASSERT(docId > 0);
> -Q_ASSERT(!url.isEmpty());
> -Q_ASSERT(!url.endsWith('/'));
> +if (!docId || url.isEmpty() || url.endsWith('/')) {
> +return false;

Would it make sense to include a warning then? If it's a wrong input, we'll 
need ways to debug it now that it won't be giving a backtrace.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: apol, sitter, kde-frameworks-devel, broulik, #frameworks, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns added a comment.
Herald added a subscriber: kde-frameworks-devel.


  Ping!

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: kde-frameworks-devel, broulik, #frameworks, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns marked 2 inline comments as done.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: broulik, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns updated this revision to Diff 33171.
bruns added a comment.


  fix variable name

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12336?vs=32529&id=33171

BRANCH
  db_robustness2

REVISION DETAIL
  https://phabricator.kde.org/D12336

AFFECTED FILES
  src/engine/documenturldb.cpp

To: bruns, #baloo, michaelh, #frameworks
Cc: broulik, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12336: Replace several Q_ASSERTs with proper checks

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> documenturldb.cpp:107
>  {
> -Q_ASSERT(id > 0);
> -Q_ASSERT(!name.isEmpty());
> +if (!docId || name.isEmpty()) {
> +return;

Didn't you mean `id`?

> documenturldb.cpp:172
>  {
> -Q_ASSERT(docId > 0);
> +if (newFileName.isEmpty() || !docId) {
> +return;

Put `docId` check first for consistency

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: broulik, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns added a reviewer: Frameworks.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D12336

To: bruns, #baloo, michaelh, #frameworks
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12336: Replace several Q_ASSERTs with proper checks

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, michaelh.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added a subscriber: Frameworks.
bruns requested review of this revision.

REVISION SUMMARY
  The code has some preconditions on supplied values when interacting
  with the DB. Instead of silently corrupting the DB with bogus values
  when using non-debug builds, check any provided arguments.

REPOSITORY
  R293 Baloo

BRANCH
  db_robustness2

REVISION DETAIL
  https://phabricator.kde.org/D12336

AFFECTED FILES
  src/engine/documenturldb.cpp

To: bruns, #baloo, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns