Bug 149215 – REGRESSION (r189526): Nightlies don't work on Mavericks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ricco]

Created attachment 261308 [details]
screen shot

I am new to webkit, but as soon as I want to try webkit. It won't even load "file:///Applications/WebKit.app/Contents/Resources/start.html". Or simply google.com.

Things I have tried:
restarting mac
updating my xcode
desktop Safari private browsing
desktop Safari un-private browsing
lots and LOTS of googling

[Alexey Proskuryakov]

*** Bug 149214 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

I can reproduce. There is an error in console saying that _sqlite3_errstr cannot be found in libsqlite3.dylib, meaning that this is a regression from <http://trac.webkit.org/changeset/189526>.

This may have something to do with how nightlies are built. On a pristine 10.9.5 installation, SQLITE_VERSION_NUMBER is 3007013, so we shouldn't be trying to compile this in. But looks like we are using the a newer SDK.

I don't know if it's feasible to change how nightlies are built. If not, we should change the conditional to use target OS version on OS X.

#if SQLITE_VERSION_NUMBER >= 3007015
            WTFLogAlways("Failed to initialize SQLite: %s", sqlite3_errstr(ret));
#else
            WTFLogAlways("Failed to initialize SQLite");
#endif

[Michael Catanzaro]

(In reply to comment #2)
> I don't know if it's feasible to change how nightlies are built. If not, we
> should change the conditional to use target OS version on OS X.

Er... wow. That's kind of terrible, but I don't see any other option, if it's hard to fix the SDK to use the lowest-supported version of SQLite. Anyway, that conditional is there exclusively for OS X, so feel free to replace that check with whatever you need.

I am not used to this sort of problem, since it would be unheard of on Linux to compile against a different version of a library than is used on the system.

[Alexey Proskuryakov]

Created attachment 261451 [details]
proposed fix

Fixing the code as outlined above. We will discuss how we build the nightlies separately.

[Michael Catanzaro]

(Tangent: the check you're removing in disableThreadingChecks() was never sufficient to do what it says it does: SQLite could still be complied to be not-threadsafe, which you'd hope users would not do but is possible, and that could also be disabled by the application by initializing SQLite before WebKit does. The reason someone might try this is to improve performance.)

[Daniel Bates]

Comment on attachment 261451 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261451&action=review

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:70
> +#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000) || (!defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && SQLITE_VERSION_NUMBER >= 3007015)

Would it make sense to write this in terms of PLATFORM(MAC) as opposed to whether __MAC_OS_X_VERSION_MIN_REQUIRED is defined?

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:378
> +    // Note that SQLite could be compiled with -DTHREADSAFE, or you may have turned off the mutexes.

I am assuming there is no way for us to ensure that SQLite was built to support sharing handles across threads. Otherwise, we should detect such support.

Maybe a better comment would be:

We assume that SQLite was compiled with -DTHREADSAFE and mutex support enabeld so as to support sharing handles across thread.

OR, more concisely,

We assume that SQLite was built to support sharing handles across thread.

[Alexey Proskuryakov]

> Would it make sense to write this in terms of PLATFORM(MAC) as opposed to whether __MAC_OS_X_VERSION_MIN_REQUIRED is defined?

Checking whether __MAC_OS_X_VERSION_MIN_REQUIRED was defined seemed more direct and thus better to me. Either one would work fine.

> Maybe a better comment would be:

These all seem fairly opaque to me, so I view the comment as a placeholder for svn blame :)

[Michael Catanzaro]

(In reply to comment #7)
> These all seem fairly opaque to me, so I view the comment as a placeholder
> for svn blame :)

For reference: https://sqlite.org/threadsafe.html

You can use sqlite3_threadsafe() to check at runtime whether enough mutexes were compiled in to make it safe to use the library from multiple threads (threadsafe level 1), but not whether it's safe to share handles across threads (threadsafe level 2, the default). I don't know any way to check that. Also, it can be changed by the application at runtime, under WebKit's back. I don't know any way to check that, either. So it doesn't appear to be possible.

[WebKit Commit Bot]

Comment on attachment 261451 [details]
proposed fix

Clearing flags on attachment: 261451

Committed r189990: <http://trac.webkit.org/changeset/189990>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.