Bug 75048

Summary: REGRESSION(r103429) - fast/workers/storage/use-same-database-in-page-and-workers.html asserts
Product: WebKit Reporter: Adrienne Walker <enne>
Component: WebCore Misc.Assignee: Hao Zheng <zhenghao>
Status: RESOLVED FIXED    
Severity: Normal CC: dslomov, enne, levin, levin+threading, michaeln, webkit.review.bot, zhenghao
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch.
none
Proposed patch 2.
none
Patch none

Description Adrienne Walker 2011-12-21 15:26:00 PST
From http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fworkers%2Fstorage%2Fuse-same-database-in-page-and-workers.html&showExpectations=true:

ERROR: Unable to turn on incremental auto-vacuum (0 not an error)
ERROR: Unable to turn on incremental auto-vacuum (0 not an error)
ERROR: Error (5) reading text result from database (SELECT value FROM __WebKitDatabaseInfoTable__ WHERE key = 'WebKitDatabaseVersionKey';)
ERROR: Failed to retrieve version from database file://::UseSameDatabaseInPageAndWorkers
ERROR: Error (9) preparing statement to read text result from database (SELECT value FROM __WebKitDatabaseInfoTable__ WHERE key = 'WebKitDatabaseVersionKey';)
ERROR: Failed to retrieve version from database file://::UseSameDatabaseInPageAndWorkers
ASSERTION FAILED: m_workerContext->hasOneRef()
Comment 1 Adrienne Walker 2011-12-21 15:38:54 PST
Committed r103466: <http://trac.webkit.org/changeset/103466>
Comment 2 Hao Zheng 2012-02-09 02:18:05 PST
The ASSERT is triggered because the test timed out and cleanup is not done properly. If timeout of DRT is set to a smaller value, the crash is easily to reproduce (e.g. change m_timeout in TestShell.cpp to 6 * 1000).
Comment 3 Hao Zheng 2012-02-13 04:52:19 PST
Created attachment 126756 [details]
Proposed patch.

I uploaded an initial patch for the crash. Code needs more polish, but I want to have it reviewed by you first in case I'm not going on the right way.

2 problems with my patch are:
- Added one variable to Task. Maybe the better way is to create a new subclass of Task, but I found it very hard to make it right, replacing createCallbackTask as much cross thread trick is involved.
- A smaller one: shouldn't use PassOwnPtr as local variable. I tried to change it as RefPtr, but it doesn't compile.

I need your helpful comment on both of them.
Comment 4 Hao Zheng 2012-02-13 05:03:27 PST
BTW, you can trigger the crash by increasing WORKER_COUNT to 5 in the test html, without changing timeout value in DRT.
Comment 5 Michael Nordman 2012-02-13 12:03:57 PST
Comment on attachment 126756 [details]
Proposed patch.

Thank you for looking into this bug.

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

> Source/WebCore/storage/Database.cpp:221
> +    m_transactionQueue.clear();

Does this call have to be made with the m_transactionInProgressMutex lock held? It's Deque which is not a thread-safe container, and all other accesses are made under that lock.
Comment 6 Dmitry Lomov 2012-02-13 12:07:45 PST
(In reply to comment #3)
> Created an attachment (id=126756) [details]
> Proposed patch.

Hao,

thank you for finding out a root cause for the issue - much appreciated!

> 
> I uploaded an initial patch for the crash. Code needs more polish, but I want to have it reviewed by you first in case I'm not going on the right way.
> 
> 2 problems with my patch are:
> - Added one variable to Task. Maybe the better way is to create a new subclass of Task, but I found it very hard to make it right, replacing createCallbackTask as much cross thread trick is involved.

Adding a variable to ScriptExecutionContext::Task is not really acceptable. Being a cleanup task is not really a settable property of a task; each class of tasks is either a clean-up task or not. See http://trac.webkit.org/browser/trunk/Source/WebCore/storage/chromium/DatabaseTrackerChromium.cpp for examples of how to derive clean-up tasks from ScriptExecutionContext::Task.

> - A smaller one: shouldn't use PassOwnPtr as local variable. I tried to change it as RefPtr, but it doesn't compile.

I think using PassOwnPtr in this situation is fine (otherwise you'd need a dance with leakPtrs), but due to my large point of not making cleanUpTak property settable the whole thing is moot. 
> 
> I need your helpful comment on both of them.

I hope the above is helpful :)
Let me know if you need further info. I'll be happy to help further.
Comment 7 David Levin 2012-02-13 12:55:33 PST
(In reply to comment #3)
> Created an attachment (id=126756) [details]
> - A smaller one: shouldn't use PassOwnPtr as local variable. I tried to change it as RefPtr, but it doesn't compile.

Use OwnPtr (not RefPtr).
Comment 8 Hao Zheng 2012-02-13 22:34:40 PST
(In reply to comment #5)
> (From update of attachment 126756 [details])
> Thank you for looking into this bug.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=126756&action=review
> 
> > Source/WebCore/storage/Database.cpp:221
> > +    m_transactionQueue.clear();
> 
> Does this call have to be made with the m_transactionInProgressMutex lock held? It's Deque which is not a thread-safe container, and all other accesses are made under that lock.

Good point.
Comment 9 Hao Zheng 2012-02-14 01:11:50 PST
Created attachment 126928 [details]
Proposed patch 2.
Comment 10 Hao Zheng 2012-02-14 01:15:32 PST
(In reply to comment #6)
> (In reply to comment #3)
> > Created an attachment (id=126756) [details] [details]
> > Proposed patch.
> 
> Hao,
> 
> thank you for finding out a root cause for the issue - much appreciated!
> 
> > 
> > I uploaded an initial patch for the crash. Code needs more polish, but I want to have it reviewed by you first in case I'm not going on the right way.
> > 
> > 2 problems with my patch are:
> > - Added one variable to Task. Maybe the better way is to create a new subclass of Task, but I found it very hard to make it right, replacing createCallbackTask as much cross thread trick is involved.
> 
> Adding a variable to ScriptExecutionContext::Task is not really acceptable. Being a cleanup task is not really a settable property of a task; each class of tasks is either a clean-up task or not. See http://trac.webkit.org/browser/trunk/Source/WebCore/storage/chromium/DatabaseTrackerChromium.cpp for examples of how to derive clean-up tasks from ScriptExecutionContext::Task.
> 

Done.

> > - A smaller one: shouldn't use PassOwnPtr as local variable. I tried to change it as RefPtr, but it doesn't compile.
> 
> I think using PassOwnPtr in this situation is fine (otherwise you'd need a dance with leakPtrs), but due to my large point of not making cleanUpTak property settable the whole thing is moot. 
> > 
> > I need your helpful comment on both of them.
> 
> I hope the above is helpful :)
> Let me know if you need further info. I'll be happy to help further.

Thanks. PTAL
Comment 11 Dmitry Lomov 2012-02-14 09:24:33 PST
Comment on attachment 126928 [details]
Proposed patch 2.

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

Looks good to me

> Source/WebCore/ChangeLog:17
> +        No new tests. (OOPS!)

Remove the OOPS (this is covered with existing tests).

> Source/WebCore/ChangeLog:20
> +        * dom/ScriptExecutionContext.h:

Remove unchanged file from ChangeLog
Comment 12 David Levin 2012-02-14 14:00:02 PST
Comment on attachment 126928 [details]
Proposed patch 2.

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

Please consider addressing the above comments and then it looks good.

> Source/WebCore/ChangeLog:25
> +        (WebCore::Database::close):

It would be nice to add a comment here about why "close" is added. I think you explained it above but it should just be removed from that big paragraph and put here instead.

> Source/WebCore/ChangeLog:27
> +        (WebCore::SQLCallbackWrapper::clear):

Explanation from above should go here and be removed from above

" turn SafeReleaseTask
        into a cleanup task, which is necessary because at the time of
        SafeReleaseTask is performed, WorkerRunLoop has been terminated
       and only runs cleanup tasks."

> Source/WebCore/storage/SQLCallbackWrapper.h:78
> +            CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))));

Just evaluate "CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))" by hand and replace this with the result.  I believe that the result is "callback"

Much simpler :).
Comment 13 Michael Nordman 2012-02-14 14:10:14 PST
Comment on attachment 126928 [details]
Proposed patch 2.

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

>> Source/WebCore/storage/SQLCallbackWrapper.h:78
>> +            CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))));
> 
> Just evaluate "CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))" by hand and replace this with the result.  I believe that the result is "callback"
> 
> Much simpler :).

yes please... simpler is better... i was wondering what that inscrutable line of code resulted in :)
Comment 14 Hao Zheng 2012-02-14 18:56:14 PST
Created attachment 127100 [details]
Patch
Comment 15 Hao Zheng 2012-02-14 18:58:08 PST
(In reply to comment #11)
> (From update of attachment 126928 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126928&action=review
> 
> Looks good to me
> 
> > Source/WebCore/ChangeLog:17
> > +        No new tests. (OOPS!)
> 

Done

> Remove the OOPS (this is covered with existing tests).
> 
> > Source/WebCore/ChangeLog:20
> > +        * dom/ScriptExecutionContext.h:
> 
> Remove unchanged file from ChangeLog

Done. I forgot to change ChangeLog.
Comment 16 Hao Zheng 2012-02-14 19:01:37 PST
(In reply to comment #12)
> (From update of attachment 126928 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126928&action=review
> 
> Please consider addressing the above comments and then it looks good.
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore::Database::close):
> 
> It would be nice to add a comment here about why "close" is added. I think you explained it above but it should just be removed from that big paragraph and put here instead.
> 

Done.

> > Source/WebCore/ChangeLog:27
> > +        (WebCore::SQLCallbackWrapper::clear):
> 
> Explanation from above should go here and be removed from above
> 
> " turn SafeReleaseTask
>         into a cleanup task, which is necessary because at the time of
>         SafeReleaseTask is performed, WorkerRunLoop has been terminated
>        and only runs cleanup tasks."
> 

Done.

> > Source/WebCore/storage/SQLCallbackWrapper.h:78
> > +            CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))));
> 
> Just evaluate "CrossThreadCopier< AllowAccessLaterWrapper<T> >::copy(AllowAccessLater(callback))" by hand and replace this with the result.  I believe that the result is "callback"
> 
> Much simpler :).

Good point. Actually, I just inlined all the old macro here to add one methd isCleanupTask(). As it's really a cross thread task, I don't realize I can inline more core here. Thanks for the comment.
Comment 17 WebKit Review Bot 2012-02-14 23:04:01 PST
Comment on attachment 127100 [details]
Patch

Clearing flags on attachment: 127100

Committed r107784: <http://trac.webkit.org/changeset/107784>
Comment 18 WebKit Review Bot 2012-02-14 23:04:06 PST
All reviewed patches have been landed.  Closing bug.