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
Created attachment 122590 [details] patch to check for syncthread in open The additional check in open, prevents creation of multiple syncThreads
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
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
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.
(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
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
Created attachment 123073 [details] DRT Logs; assert avoided; we detected syncThread running
Created attachment 123074 [details] DRT Logs; assert hit; attempt to OpenDB from multiple threads
Brady Eidson, Can you please let me know how i can close this bug ?
(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.
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 ?
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.
(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
Created attachment 128380 [details] Patch with why no LayoutTest explanation I have added explanation as per Eric's suggestion
Eric/Brady, Can this be landed (or) Is it mandatory to add the LayoutTest ? Thanks sriram
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)?
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?
(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
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?
(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
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.