Bug 75048 – REGRESSION(r103429) - fast/workers/storage/use-same-database-in-page-and-workers.html asserts

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

[Adrienne Walker]

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()

[Adrienne Walker]

Committed r103466: <http://trac.webkit.org/changeset/103466>

[Hao Zheng]

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).

[Hao Zheng]

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.

[Hao Zheng]

BTW, you can trigger the crash by increasing WORKER_COUNT to 5 in the test html, without changing timeout value in DRT.

[Michael Nordman]

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.

[Dmitry Lomov]

(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.

[David Levin]

(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).

[Hao Zheng]

(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.

[Hao Zheng]

Created attachment 126928 [details]
Proposed patch 2.

[Hao Zheng]

(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

[Dmitry Lomov]

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

[David Levin]

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 :).

[Michael Nordman]

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 :)

[Hao Zheng]

Created attachment 127100 [details]
Patch

[Hao Zheng]

(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.

[Hao Zheng]

(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.

[WebKit Review Bot]

Comment on attachment 127100 [details]
Patch

Clearing flags on attachment: 127100

Committed r107784: <http://trac.webkit.org/changeset/107784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.