Bug 149215 - REGRESSION (r189526): Nightlies don't work on Mavericks
Summary: REGRESSION (r189526): Nightlies don't work on Mavericks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac OS X 10.9
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
: 149214 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-09-16 09:01 PDT by Ricco
Modified: 2015-10-25 16:15 PDT (History)
7 users (show)

See Also:


Attachments
screen shot (83.50 KB, image/png)
2015-09-16 09:01 PDT, Ricco
no flags Details
proposed fix (2.87 KB, patch)
2015-09-17 16:20 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ricco 2015-09-16 09:01:22 PDT
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
Comment 1 Alexey Proskuryakov 2015-09-16 22:56:16 PDT
*** Bug 149214 has been marked as a duplicate of this bug. ***
Comment 2 Alexey Proskuryakov 2015-09-16 23:20:54 PDT
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
Comment 3 Michael Catanzaro 2015-09-17 05:32:37 PDT
(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.
Comment 4 Alexey Proskuryakov 2015-09-17 16:20:33 PDT
Created attachment 261451 [details]
proposed fix

Fixing the code as outlined above. We will discuss how we build the nightlies separately.
Comment 5 Michael Catanzaro 2015-09-18 04:44:42 PDT
(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.)
Comment 6 Daniel Bates 2015-09-18 11:54:40 PDT
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.
Comment 7 Alexey Proskuryakov 2015-09-18 12:51:00 PDT
> 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 :)
Comment 8 Michael Catanzaro 2015-09-18 13:00:29 PDT
(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.
Comment 9 WebKit Commit Bot 2015-09-18 15:37:09 PDT
Comment on attachment 261451 [details]
proposed fix

Clearing flags on attachment: 261451

Committed r189990: <http://trac.webkit.org/changeset/189990>
Comment 10 WebKit Commit Bot 2015-09-18 15:37:14 PDT
All reviewed patches have been landed.  Closing bug.