Bug 75048

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

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

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.

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

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.

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

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

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

Created attachment 126928 [details] Proposed patch 2.

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

Created attachment 127100 [details] Patch

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

(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 on attachment 127100 [details] Patch Clearing flags on attachment: 127100 Committed r107784: <http://trac.webkit.org/changeset/107784>

All reviewed patches have been landed. Closing bug.