Closed Bug 1470961 Opened 2 years ago Closed 2 years ago

build failure with system-sqlite in netwerk/cache

Categories

(Toolkit :: Storage, defect, P3)

Unspecified
OpenBSD
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: gaston, Assigned: mak)

Details

Attachments

(1 file)

Noticed since a while, forgot to report it, and hit it again.

using --with-system-sqlite, the build fails on netwerk/cache/nsDiskCacheDeviceSQL.cpp

In file included from /build/obj/ports/firefox-esr-60.1.0rc2/build-i386/netwerk/cache/Unified_cpp_netwerk_cache0.cpp:110:                                      
In file included from /build/obj/ports/firefox-esr-60.1.0rc2/firefox-60.1.0/netwerk/cache/nsDiskCacheDeviceSQL.cpp:51:                                        
/build/obj/ports/firefox-esr-60.1.0rc2/build-i386/dist/system_wrappers/sqlite3.h:3:15: fatal error: 'sqlite3.h' file not found                                
#include_next <sqlite3.h>                                                                                                                                     
              ^~~~~~~~~~~

sqlite is installed in /usr/local/include, configure detects it fine:
  'MOZ_SYSTEM_SQLITE': '1',
    'SQLITE_CFLAGS': [
        '-I/usr/local/include',
    ],
    'SQLITE_LIBS': [
        '-L/usr/local/lib',
        '-lsqlite3',
    ],

but the build ultimately fails. I had worked around it since fx 56 by putting CPPFLAGS=-I/usr/local/include in the environment during configure; but that's ugly, and i just hit it again because i had forgotten this workaround.

Maybe netwerk/cache/moz.build should add SQLITE_CFLAGS to CXXFLAGS and/or CFLAGS like  storage/moz.build, storage/build/moz.build and toolkit/components/url-classifier/moz.build do ?

Wasnt those --with-system flags were supposed to be tested by TC at some point ?
CXXFLAGS += CONFIG['SQLITE_CFLAGS'] in netwerk/cache/moz.build fixes the issue, i no longer have to set CPPFLAGS=-I/usr/local/include in the environment during configure
It seems sqlite3.h is only used for SQLITE_IGNORE, and it seems to me this should be abstracted in the mozIStorage layer.
Component: Build Config → Storage
That code looks strange, maybe I'm misreading it, but the SQL function seems to be setup to just return SQLITE_IGNORE (that is integer 2)
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/netwerk/cache/nsDiskCacheDeviceSQL.cpp#221

The code added to Storage int retcode = variantToSQLiteT(aCtx, result); won't actually return SQLITE_IGNORE, but instead will add an integer result with value 2 and return SQLITE_OK.
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/storage/mozStorageConnection.cpp#263

It looks like the if (SQLITE_IGNORE) code path is just never hit, and even if it would, variantToSQLiteT is already doing a ::sqlite3_result_int(aCtx, aValue); so what would be the point of doing that again?

Apparently we can just remove the if (SQLITE_IGNORE) code from mozStorageConnection and just early return in nsDiskCacheDeviceSQL.cpp with a NullVariant();
Priority: -- → P3
## Context digging
In https://bugzilla.mozilla.org/show_bug.cgi?id=767025#c70, :gal said:
"""
::: storage/src/mozStorageConnection.cpp
@@ +211,5 @@
>                             -1);
>      return;
>    }
> +  int retcode = variantToSQLiteT(aCtx, result);
> +  if (retcode == SQLITE_IGNORE) {

I verified that "SQLITE_IGNORE" isn't referenced in the code base before this patch (outside sqlite itself), so this change has zero risk.
"""

And :gal dropped the review request against :mak in https://bugzilla.mozilla.org/show_bug.cgi?id=767025#c85 with prescient foresight that someone might eventually complain.

## Original fix intent

I understand the point of the patches on the bug 767025 was to:
- Stop tracking whether an entry was in use on disk and instead track it in memory.  This flag was used to prevent deletion of active entries:
  - Pre-patch SQL: "DELETE FROM moz_cache WHERE ClientID=? AND Flags = 0;"
  - Post-patch SQL: "DELETE FROM moz_cache WHERE ClientID=?;"
- The logic was intended to be changed so that the pre-existing cache_eviction_observer() custom SQL function which was used in a BEFORE DELETE TRIGGER would cause the deletion to abort and not be processed.

## Reality

As :mak notes, the changes around SQLITE_IGNORE are super confusing.  As I understand the documentation at https://www.sqlite.org/lang_createtrigger.html#raise, the correct thing to do in such a trigger would be to invoke the RAISE function with IGNORE.  It looks like the patch thought that this was what it was doing, but it was not.  I also don't see any documentation or tests or implementation logic in SQLite's source that would suggest that returning a specific value or null would result in the deletion being skipped.

It seems like, to the extent the logic changes worked, they worked because nsOfflineCacheEvictionFunction::OnFunctionCall returned before storing them in the items array.  By not storing them in the array, they would never have Remove() called on their file.  This seems to be all that the test landed at the time cared about.  I think if the test would have been more thorough, it would have found that the cache items end up leaking because the row is deleted so the eviction function will never get a second chance to successfully put the item in the array when it should be eligible for eviction.

I think the reasonable course of action is to land this cleanup patch and make sure necko is aware that their code wasn't working, probably in the form of a spin-off bug.  To that end, :mayhemer, I'm needinfo-ing you because of your original involvement so you're aware of what we're doing here, and because perhaps you can file a more useful follow-up than we would file.  I think the code in question actually is for appcache which we're trying to deprecate, so maybe the whole issue is moot-ish?
Flags: needinfo?(honzab.moz)
Comment on attachment 8987808 [details]
Bug 1470961 - Remove unnecessary use of SQLITE_IGNORE.

https://reviewboard.mozilla.org/r/253100/#review259720

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
(Diff revision 1)
>    fullKey.Append(key);
>    int generation  = values->AsInt32(2);
>  
>    // If the key is currently locked, refuse to delete this row.
>    if (mDevice->IsLocked(fullKey)) {
> -    NS_ADDREF(*_retval = new IntegerVariant(SQLITE_IGNORE));

It might be appropriate to add a comment here like:
This code thought it was performing the equivalent of invoking the SQL "RAISE(IGNORE)" function.  It was not.  Please see bug 1470961 and any follow-ups to understand the plan for correcting this bug.
Attachment #8987808 - Flags: review?(bugmail) → review+
Shouldnt the sqlite3.h inclusion be removed too ?
Bah, sorry, read the wrong review chunk. disregard.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/bd92ec274e39
Remove unnecessary use of SQLITE_IGNORE. r=asuth
https://hg.mozilla.org/mozilla-central/rev/bd92ec274e39
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.