Bug 76362

Summary:

ASSERT_ICON_SYNC_THREAD fired in WebCore::IconDatabase::iconDatabaseSyncThread()

Product:

WebKit

Reporter:

Sriram Neelakandan <sriram.neelakandan>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

beidson, eric, japhet, mrowe, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch to check for syncthread in open	eric: review-
DRT Logs; assert avoided; we detected syncThread running	none
DRT Logs; assert hit; attempt to OpenDB from multiple threads	none
Patch with why no LayoutTest explanation	none

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-	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.