Bug 50825 – GeolocationPositionCache should do database access on background thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

GeolocationPositionCache should do database access on background thread

[Steve Block]

Created attachment 76878 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76878 [details]
Patch

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

> WebCore/page/GeolocationPositionCache.cpp:38
>  

no newline

> WebCore/page/GeolocationPositionCache.cpp:64
> +    if (!(numUsers++) && !m_threadId) {

This one line is doing a lot.  Maybe the ++numUsers should stay outside of the if statement?

> WebCore/page/GeolocationPositionCache.cpp:75
>      if (!(--numUsers) && m_cachedPosition)

ditto

> WebCore/page/GeolocationPositionCache.cpp:106
> +    m_threadId = createThread(threadEntryPoint, this, "WebCore: GeolocationPositionCache");

Is there any other thread we can share?  Or will this be used for anything else?

> WebCore/page/GeolocationPositionCache.cpp:118
> +        task->performTask(0 /* ScriptExecutionContext */);

I don't think we typically use /* style comments.  Maybe instead of saying it's the SEC (which anyone can look up) you should explain why it's fine to pass 0...i.e. it's unused on the other side.  Or just delete.

> WebCore/page/GeolocationPositionCache.cpp:133
> +    MutexLocker lock(m_cachedPositionMutex);

Everything in this can block the main thread.

The next line is just an optimization anyway (if you do a final check before setting m_cachedPostion below).  In non-testing, things will only be created/set in this thread.  So I don't think you need the lock until my comment below.

> WebCore/page/GeolocationPositionCache.cpp:173
>      m_cachedPosition = Geoposition::create(coordinates.release(), statement.getColumnInt64(7)); // timestamp

Here you should take the lock...and do an if statement to verify it hasn't been set.

> WebCore/page/GeolocationPositionCache.cpp:188
> +    MutexLocker lock(m_cachedPositionMutex);

Only lock to get a snapshot.  Don't hold it.  Otherwise, there's no point...

[Steve Block]

> no newline
Will do

> This one line is doing a lot.  Maybe the ++numUsers should stay outside of the if statement?
Will do

> Is there any other thread we can share?  Or will this be used for anything else?
I don't have any plans to use this thread for anything else. Is there a precedent for using a thread for multiple tasks? Do you have a suggestion as to which thread I could use.

> I don't think we typically use /* style comments.
Will do

> Everything in this can block the main thread.
> Only lock to get a snapshot.  Don't hold it.  Otherwise, there's no point...
The reason I lock early is that I also need to protect access to m_databaseFile (I admit my mutex is badly named). There's still benefit, as the WebCore thread can now treat this as a 'fire and forget' operation. It's unlikely that the DB thread will be in the middle of a read/write when the WebCore thread next asks for the cached position.

One solution would be to use multiple mutexes and use finer grained locking, but that adds more complexity. WDYT?

[Jeremy Orlow]

(In reply to comment #3)
> > no newline
> Will do
> 
> > This one line is doing a lot.  Maybe the ++numUsers should stay outside of the if statement?
> Will do
> 
> > Is there any other thread we can share?  Or will this be used for anything else?
> I don't have any plans to use this thread for anything else. Is there a precedent for using a thread for multiple tasks? Do you have a suggestion as to which thread I could use.

There's not much precedent for threads in general.  In the few places that use them, it's one function per thread.  So I guess leave it, but maybe put a FIXME combine with other threads?  or something.
 
> > I don't think we typically use /* style comments.
> Will do
> 
> > Everything in this can block the main thread.
> > Only lock to get a snapshot.  Don't hold it.  Otherwise, there's no point...
> The reason I lock early is that I also need to protect access to m_databaseFile (I admit my mutex is badly named). There's still benefit, as the WebCore thread can now treat this as a 'fire and forget' operation. It's unlikely that the DB thread will be in the middle of a read/write when the WebCore thread next asks for the cached position.
> 
> One solution would be to use multiple mutexes and use finer grained locking, but that adds more complexity. WDYT?

Just use one mutex and {}'s to control the scoping of it.  You can lock it to get the data, do the read, and then lock again to set the values (and verify the constraints are still as expected...if not, I guess loop and run again?).

[Steve Block]

Created attachment 76999 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76999 [details]
Patch

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

> WebCore/page/GeolocationPositionCache.cpp:221
> +    MutexLocker lock(m_cachedPositionMutex);

You probably should just save the data off while you hold the lock above.

[Early Warning System Bot]

Attachment 76999 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7318068

[Steve Block]

Created attachment 77001 [details]
Patch

[Steve Block]

Created attachment 77002 [details]
Patch

[Jeremy Orlow]

Comment on attachment 77001 [details]
Patch

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

> WebCore/page/GeolocationPositionCache.cpp:206
> +        cachedPosition = m_cachedPosition;

You can't do this.  Geoposition is not threadsafe ref counted.  You'll need to copy all the individual values or add a method to get a copy.

[Eric Seidel (no email)]

Attachment 77001 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7201074

[Build Bot]

Attachment 76999 [details] did not build on win:
Build output: http://queues.webkit.org/results/7198090

[WebKit Review Bot]

Attachment 76999 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7241075

[Steve Block]

Created attachment 77011 [details]
Patch

[Jeremy Orlow]

Comment on attachment 77011 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 77011 [details]
Patch

Rejecting attachment 77011 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 77011]" exit_code: 2
Last 500 characters of output:
ations.txt
Failed to merge in the changes.
Patch failed at 0001 2010-12-20  Yury Semikhatsky  <[e-mail]>

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 132.

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/7297070

[Steve Block]

Committed r74354: <http://trac.webkit.org/changeset/74354>

[David Levin]

Comment on attachment 77011 [details]
Patch

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

> WebCore/page/GeolocationPositionCache.cpp:102
> +    MutexLocker lock(m_cachedPositionMutex);
>      return m_cachedPosition.get();

This is clearly wrong.

> WebCore/page/GeolocationPositionCache.cpp:121
> +        task->performTask(0);

This really isn't correct. This has nothing to do with ScriptExecutionContext.

Really what should happen is a better refactoring of this functionality but this is just hacked in.

[Eric Seidel (no email)]

I'm very sorry teh commit-queue went crazy again (corrupted its git repo).  Fixing it now.

[Simon Fraser (smfr)]

This caused bug 51557.

[Simon Fraser (smfr)]

Please assign to yourself bugs that you fix so that regressions can be blamed to the right person.

[Steve Block]

> This caused bug 51557.
It looks like this is not the cause of that bug.

[Steve Block]

> > WebCore/page/GeolocationPositionCache.cpp:102
> > +    MutexLocker lock(m_cachedPositionMutex);
> >      return m_cachedPosition.get();
> This is clearly wrong.
Can you elaborate?

> > WebCore/page/GeolocationPositionCache.cpp:121
> > +        task->performTask(0);
> 
> This really isn't correct. This has nothing to do with ScriptExecutionContext.
I'm not aware of another generic task queue that I could make use of and I was advised to make use of this class.  Is there another class I should be using? Or I could file a bug to factor out the part of this class that isn't specific to ScriptExecutionContext.

[David Levin]

(In reply to comment #23)
> > > WebCore/page/GeolocationPositionCache.cpp:102
> > > +    MutexLocker lock(m_cachedPositionMutex);
> > >      return m_cachedPosition.get();
> > This is clearly wrong.
> Can you elaborate?

This adds a lock around m_cachedPosition. Why?

Either the lock isn't needed or the returned value may be garbage (because the lock is released). Either way "this is clearly wrong." :) 


> 
> > > WebCore/page/GeolocationPositionCache.cpp:121
> > > +        task->performTask(0);
> > 
> > This really isn't correct. This has nothing to do with ScriptExecutionContext.
> I'm not aware of another generic task queue that I could make use of and I was advised to make use of this class.  Is there another class I should be using? Or I could file a bug to factor out the part of this class that isn't specific to ScriptExecutionContext.

The refactoring should have occurred to support this work beforehand rather than hacking something in like this. In such a large codebase everyone has to make efforts to not degrade it (like this) or you end up with a mess. That being said I think Kinuko is making progress in this area.

I called this out because in my opinion that is the point of code reviews to help us to not take the shortcuts that seem convenient to the developer but degrade the codebase with hacks (which I also have been called out on in code reviews).

Perhaps there should be a bug to clean this code up though because this is ugly.

[Steve Block]

> This adds a lock around m_cachedPosition. Why?
> 
> Either the lock isn't needed or the returned value may be garbage (because
> the lock is released). Either way "this is clearly wrong." :) 
This method is called on the WebCore thread. The lock is required because m_cachedPosition can be set from readFromDatabase() on the background thread.

However, once m_cachedPosition has been set to a non-null value it's never modified on the background thread, only on the WebCore thread. So the pointer returned by cachedPosition() will never become garbage due to the background thread, even after the lock is released.

> Perhaps there should be a bug to clean this code up though because this is
> ugly.
OK, filed Bug 51818

[David Levin]

(In reply to comment #25)
> > This adds a lock around m_cachedPosition. Why?
> > 
> > Either the lock isn't needed or the returned value may be garbage (because
> > the lock is released). Either way "this is clearly wrong." :) 
> This method is called on the WebCore thread. The lock is required because m_cachedPosition can be set from readFromDatabase() on the background thread.
> 
> However, once m_cachedPosition has been set to a non-null value it's never modified on the background thread, only on the WebCore thread. So the pointer returned by cachedPosition() will never become garbage due to the background thread, even after the lock is released.

ok, so this is guarding against a half-assigned pointer. Thanks.