Bug 51557

Summary:

Visiting macnn.com often causes SQL spew via geolocation database

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

WebCore Misc.

Assignee:

Steve Block <steveblock>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, jorlow, levin, sam, simon.fraser, steveblock

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

65289

Attachments:

Description	Flags
Patch	none

Simon Fraser (smfr)

Reported 2010-12-23 12:59:25 PST

Sometimes, when loading macnn.com, I get this output on the console: ERROR: SQLite database failed to load from Cause - out of memory (/OpenSource/WebCore/platform/sql/SQLiteDatabase.cpp:70 bool WebCore::SQLiteDatabase::open(const WTF::String&, bool)) This is coming out of geolocation code: #0 WebCore::SQLiteDatabase::open (this=0x13739ad20, filename=@0x10bcc9008, forWebSQLDatabase=false) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/platform/sql/SQLiteDatabase.cpp:71 #1 0x000000010320e81c in WebCore::GeolocationPositionCache::readFromDatabaseImpl (this=0x10bcc8f10) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/page/GeolocationPositionCache.cpp:140 #2 0x000000010320ecb9 in WebCore::GeolocationPositionCache::readFromDatabase (cache=0x10bcc8f10) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/page/GeolocationPositionCache.cpp:132 #3 0x000000010320f504 in WebCore::CrossThreadTask1<WebCore::GeolocationPositionCache*, WebCore::GeolocationPositionCache*>::performTask (this=0x10bccd4c0, context=0x0) at CrossThreadTask.h:81 #4 0x000000010320ed29 in WebCore::GeolocationPositionCache::threadEntryPointImpl (this=0x10bcc8f10) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/page/GeolocationPositionCache.cpp:121 #5 0x000000010320ed57 in WebCore::GeolocationPositionCache::threadEntryPoint (object=0x10bcc8f10) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/page/GeolocationPositionCache.cpp:113 #6 0x0000000101a8d149 in WTF::threadEntryPoint (contextData=0x10bcbb0b0) at /Volumes/InternalData/Development/webkit/OpenSource/JavaScriptCore/wtf/Threading.cpp:65 filename is a null string. So: 1. Why is getting window.geolocatiaon spawning a thread and trying to open a database? 2. Why is the database path empty? 3. Why is the SQL error so totally bogus?

Attachments
Patch (2.96 KB, patch) 2010-12-29 11:45 PST, Steve Block	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2010-12-23 12:59:44 PST

<rdar://problem/8800389>

Steve Block

Comment 2 2010-12-23 13:56:43 PST

Apologies. This was caused by http://trac.webkit.org/changeset/74354 I'll look into this ASAP, but please feel free to roll back my change if required.

Steve Block

Comment 3 2010-12-29 11:42:51 PST

> 1. Why is getting window.geolocatiaon spawning a thread and trying to open a database? This thread is to handle reading from and writing to the database used to cache Geolocation positions. We start the thread as soon as the navigator.geolocation object is accessed so that it's ready when Geolocation is used. > 2. Why is the database path empty? This path can be set by calling GeolocationPositionCache::setDatabasePath(). It seems this is not called by the Mac port (Geolocation will work even without a cache). Prior to the refactoring in http://trac.webkit.org/changeset/74226, if the path is not set we early-out when trying to open the DB. Since that refactoring, if the path is not set we use an empty path so the DB open fails. I'll send a patch to restore the early-out behaviour to avoid the log spew. > 3. Why is the SQL error so totally bogus? I have no idea. I don't think this is specific to Geolocation. > Apologies. This was caused by http://trac.webkit.org/changeset/74354 It seems this is not the cause after all.

Steve Block

Comment 4 2010-12-29 11:45:13 PST

Created attachment 77634 [details] Patch

Sam Weinig

Comment 5 2010-12-29 14:24:15 PST

Comment on attachment 77634 [details] Patch Why are we hitting the cache when just fetching the geolocation object. This seems silly since people often fetch the geolocation object in feature detection, even if they are not using it. r=me.

Steve Block

Comment 6 2010-12-30 02:53:13 PST

> Why are we hitting the cache when just fetching the geolocation object. The idea is to start the DB thread as soon as navigator.geolocation is accessed. Then when a Geolocation method is called, we potentially use a cached position if one is available, but we don't wait for the cache. If we don't start the DB thread until a Geolocation method is called, the cache will almost certainly never be used for this first method call. > This > seems silly since people often fetch the geolocation object in feature > detection, even if they are not using it. Really, do you think it's likely that people feature detect for a feature they don't plan to use? An alternative is to change the logic such that the DB thread is not started until a Geolocation method is called, but then have the Geolocation object wait for a cache response before instructing the GeolocationController to do work to get a position fix. This is a more involved change which could complicate the logic considerably. I'll look into it.

WebKit Commit Bot

Comment 7 2010-12-30 03:32:31 PST

Comment on attachment 77634 [details] Patch Clearing flags on attachment: 77634 Committed r74794: <http://trac.webkit.org/changeset/74794>

WebKit Commit Bot

Comment 8 2010-12-30 03:32:37 PST

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

Comment 9 2010-12-31 09:09:40 PST

(In reply to comment #3) > > 1. Why is getting window.geolocatiaon spawning a thread and trying to open a database? > This thread is to handle reading from and writing to the database used to cache Geolocation positions. We start the thread as soon as the navigator.geolocation object is accessed so that it's ready when Geolocation is used. I agree with Sam that this should not happen. Please file a bug on this too. > > 2. Why is the database path empty? > This path can be set by calling GeolocationPositionCache::setDatabasePath(). It seems this is not called by the Mac port Please file a bug on that.

Steve Block

Comment 10 2011-01-03 03:25:45 PST

> I agree with Sam that this should not happen. Please file a bug on this too. Filed Bug 51814 > > This path can be set by calling GeolocationPositionCache::setDatabasePath(). > > It seems this is not called by the Mac port > Please file a bug on that. Filed Bug 51813.

Note You need to log in before you can comment on or make changes to this bug.