UNCONFIRMED 76362
ASSERT_ICON_SYNC_THREAD fired in WebCore::IconDatabase::iconDatabaseSyncThread() 
https://bugs.webkit.org/show_bug.cgi?id=76362
Summary ASSERT_ICON_SYNC_THREAD fired in WebCore::IconDatabase::iconDatabaseSyncThread()
Sriram Neelakandan
Reported 2012-01-15 21:27:24 PST
when WebCore::IconDatabase::open(const String& directory, const String& filename) is called in quick successions,multiple syncThreads are started. First thread has not been scheduled yet to mark isOpen(); Need to ensure ::open fails if m_syncThreadRunning will attach a patch for the same
Attachments
patch to check for syncthread in open (1.45 KB, patch)
2012-01-15 21:35 PST, Sriram Neelakandan 		eric: review-
DetailsFormatted DiffDiff
DRT Logs; assert avoided; we detected syncThread running (137.72 KB, text/plain)
2012-01-18 21:52 PST, Sriram Neelakandan 		no flags
Details
DRT Logs; assert hit; attempt to OpenDB from multiple threads (23.78 KB, text/plain)
2012-01-18 21:53 PST, Sriram Neelakandan 		no flags
Details
Patch with why no LayoutTest explanation (1.65 KB, patch)
2012-02-22 20:38 PST, Sriram Neelakandan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sriram Neelakandan
Comment 1 2012-01-15 21:35:45 PST
Created attachment 122590 [details] patch to check for syncthread in open The additional check in open, prevents creation of multiple syncThreads
Hajime Morrita
Comment 2 2012-01-15 22:03:28 PST
Comment on attachment 122590 [details] patch to check for syncthread in open Thanks for taking this. Could you add a layout test to cover this error? Following pages explain how to write it in detail: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree http://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches
Sriram Neelakandan
Comment 3 2012-01-15 22:29:04 PST
I am not sure how to create a LayoutTest for this case; The bug would arise for instance if an implentation calls WebCore::iconDatabase().open() multiple times For example: consecutive calls to "QWebSettings::setIconDatabasePath()" in my Qt-TestBrowser fires the ASSERT Can DRT be used to validate APIs as well ? Please let me know
Hajime Morrita
Comment 4 2012-01-16 02:22:47 PST
Comment on attachment 122590 [details] patch to check for syncthread in open Good point. This cannot be tested by DRT. Back to r?. My feeling is that isOpen() should also check m_syncThread or something. But I'm not an expert in this area.
Brady Eidson
Comment 5 2012-01-17 09:24:26 PST
(In reply to comment #3) > I am not sure how to create a LayoutTest for this case; > The bug would arise for instance if an implentation calls > WebCore::iconDatabase().open() multiple times > > For example: consecutive calls to "QWebSettings::setIconDatabasePath()" in my Qt-TestBrowser fires the ASSERT > > Can DRT be used to validate APIs as well ? Please let me know Yes. We add LayoutTestController methods that exercise the specific API in question. There's a couple methods prefixed with "apiTest" but plenty of others are also to twiddle specific APIs
Sriram Neelakandan
Comment 6 2012-01-18 21:49:16 PST
I am able to hit the Assert with the following LayoutTest html <html> <head> <script> function runTest() { if (window.layoutTestController) { layoutTestController.dumpAsText(); layoutTestController.dumpResourceLoadCallbacks(); layoutTestController.setIconDatabaseEnabled(true); var i=100; while (i--) { layoutTestController.setIconDatabaseEnabled(false); layoutTestController.setIconDatabaseEnabled(true); layoutTestController.setIconDatabaseEnabled(true); layoutTestController.setIconDatabaseEnabled(true); layoutTestController.setIconDatabaseEnabled(false); layoutTestController.setIconDatabaseEnabled(true); layoutTestController.setIconDatabaseEnabled(true); } layoutTestController.disableImageLoading(); layoutTestController.queueReload(); } } </script> <link rel="icon" href="resources/favicon.ico" type="image/x-icon"> </head> <body onload="runTest()"> This will test multiple IconDatabase Sync thread running </body> </html> And even with this i get it just once .. on my desktop see line number 174 in attached assert_avoided_success.txt The ASSERT would other wise trigger (see assert_hit_fail.txt) Now, I need some help with the LayoutTest ... I noticed that LTC[Qt].setIconDatabaseEnabled(bool) calls WebCore::iconDatabase().open() / WebCore::iconDatabase().close() and hence i used the same for testing the case. I feel that this test may become port specific.. but the actual change is port agnostic So should we add new LTC.openIconDatabase() and LTC.closeIconDatabase() APIs for testing this case ? Kindly Advice
Sriram Neelakandan
Comment 7 2012-01-18 21:52:07 PST
Created attachment 123073 [details] DRT Logs; assert avoided; we detected syncThread running
Sriram Neelakandan
Comment 8 2012-01-18 21:53:01 PST
Created attachment 123074 [details] DRT Logs; assert hit; attempt to OpenDB from multiple threads
Sriram Neelakandan
Comment 9 2012-01-23 21:51:08 PST
Brady Eidson, Can you please let me know how i can close this bug ?
Brady Eidson
Comment 10 2012-01-23 23:48:33 PST
(In reply to comment #6) > Now, I need some help with the LayoutTest ... > > I noticed that LTC[Qt].setIconDatabaseEnabled(bool) calls WebCore::iconDatabase().open() / WebCore::iconDatabase().close() > and hence i used the same for testing the case. > > I feel that this test may become port specific.. but the actual change is port agnostic > So should we add new LTC.openIconDatabase() and LTC.closeIconDatabase() APIs for testing this case ? If you wanted you could even add a LTC method designed specifically to twiddle this preference as many times as it takes to semi-reliably trigger the bug.
Sriram Neelakandan
Comment 11 2012-01-25 01:54:31 PST
Brady(In reply to comment #10) > If you wanted you could even add a LTC method designed specifically to twiddle this preference as many times as it takes to semi-reliably trigger the bug. On my desktop it happens once per 1000 iterations (depending on the system load) But on my target get it every 2 iteration Desktop = Intel(R) Core(TM) i7-2600 CPU @ 3.40GHz Target = MIPS Core @ 500 MHz Now, i am not sure about the semi-reliable way to ensure it will hit on a more powerful build-bot Is the LayoutTest mandatory for this simple check ? Can it be waived off, Please ?
Eric Seidel (no email)
Comment 12 2012-02-16 13:32:49 PST
Comment on attachment 122590 [details] patch to check for syncthread in open View in context: https://bugs.webkit.org/attachment.cgi?id=122590&action=review > Source/WebCore/ChangeLog:7 > + No new tests. (OOPS!) This line will cause the commit-queue to fail. It should be replaced by an explanation of why LayoutTesting is impractiacal for this change. Sometimes it is OK to not have tests, but we have to explain in the changelog.
Sriram Neelakandan
Comment 13 2012-02-17 10:22:31 PST
(In reply to comment #12) > > Source/WebCore/ChangeLog:7 > > + No new tests. (OOPS!) > > This line will cause the commit-queue to fail. Thanks Eric; Can I edit the change log as follows : No reliable way of recreating this using a Test; see webkit.org/b/76362#c11
Sriram Neelakandan
Comment 14 2012-02-22 20:38:22 PST
Created attachment 128380 [details] Patch with why no LayoutTest explanation I have added explanation as per Eric's suggestion
Sriram Neelakandan
Comment 15 2012-03-17 20:37:03 PDT
Eric/Brady, Can this be landed (or) Is it mandatory to add the LayoutTest ? Thanks sriram
Eric Seidel (no email)
Comment 16 2012-04-15 21:43:09 PDT
Comment on attachment 128380 [details] Patch with why no LayoutTest explanation So does this mean that we have existing LayoutTests with this race condition? That this change will now make them flaky in release builds (previously they were flaky-crashy in Debug builds)?
Eric Seidel (no email)
Comment 17 2012-04-15 21:44:21 PDT
It looks like you earlier suggested a layout test (and noted that you were able to reproduce the ASSERT there). Why wouldn't we add that layout test?
Sriram Neelakandan
Comment 18 2012-04-15 22:04:35 PDT
(In reply to comment #17) > It looks like you earlier suggested a layout test (and noted that you were able to reproduce the ASSERT there). Why wouldn't we add that layout test? Here are the reasons for not adding the LT 1. The test is not a guaranteed hit (see Comment#11) 2. The test, i added was built on the prior knowledge of QtPort/SetIconDatabaseEnabled() that will expose the bug; Not sure if this will be the case with other ports as well (see Comment#6) Basically, I am not aware of a port-agnostic test to re-create the bug in a reliable way
Eric Seidel (no email)
Comment 19 2012-04-15 22:11:39 PDT
Comment on attachment 128380 [details] Patch with why no LayoutTest explanation So there is no way to call this function directly from JavaScript, correct? What will happen to callers when this returns false? Do they all do the right thing?
Sriram Neelakandan
Comment 20 2012-05-22 23:33:31 PDT
(In reply to comment #19) > (From update of attachment 128380 [details]) What will happen to callers when this returns false? Do they all do the right thing? I checked the latest git source as of r118125 We have mixed bag of implementations, but mostly if IconDatabse::open() fails, nothing much is being done !! Not sure if implementations/users have assumed the API to never fail. Should we add some wait and re-open logic inside the API ? Note, the failure is still very rare assuming you call open/close in quick successions. 1. WebKit2/UIProcess/WebIconDatabase.cpp: void WebIconDatabase::setDatabasePath(const String &path) LOGs and no indication to caller 2. Blackberry ./Source/WebKit/blackberry/WebCoreSupport/IconDatabaseClientBlackBerry.cpp bool IconDatabaseClientBlackBerry::initIconDatabase(const BlackBerry::WebKit::WebSettings* settings) Indicates failure to caller (bool) 3. GTK ./Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp: void webkit_favicon_database_set_path(WebKitFaviconDatabase* database, const gchar* path) ./Source/WebKit/gtk/webkit/webkiticondatabase.cpp: void webkit_icon_database_set_path(WebKitIconDatabase* database, const gchar* path) Both cases no indication to caller 4. Qt ./Source/WebKit/qt/Api/qwebsettings.cpp: void QWebSettings::setIconDatabasePath(const QString& path) No indication to caller 5. EFL ./Source/WebKit/efl/ewk/ewk_settings.cpp Eina_Bool ewk_settings_icon_database_path_set(const char* directory) No indication to caller
Anders Carlsson
Comment 21 2014-02-05 10:52:20 PST
Comment on attachment 128380 [details] Patch with why no LayoutTest explanation Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Note You need to log in before you can comment on or make changes to this bug.