Currently creating WKPreference doesn't initialize the threading (JSC::initializeThreading() and WTF::initializeMainThread()). This needs to be fixed, as WKPreference might be called independently.
Created attachment 111043 [details] Skip TestWebKitSettings test
(In reply to comment #1) > Created an attachment (id=111043) [details] > Skip TestWebKitSettings test Sorry, by mistake attached a wrong patch to wrong bug!!
Created attachment 111053 [details] Initialize threading while creating WKPreference
Comment on attachment 111053 [details] Initialize threading while creating WKPreference I don’t think this is sufficient. What if the first thing I do is create some other kind of object, say by calling WKUserContentURLPatternCreate? I don’t think preferences stands out as the one entry point we missed.
Not sure if this is useful information or not, but at some point we looked at doing this automatically in WebKit1 GTK+ using shared library constructors. Eventually we decided that the risks of running afoul of linker ordering were too great and we abandoned it.
Created attachment 111220 [details] Initialize threading. This method uses shared library constructor logic to initialize threading (Thanks to Martin for the pointers). This will ensure that threading gets initialized in all code paths. As per the GCC documentation http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html, this constructor is guaranteed to get called prior to calling application main function. Currently makefile changes are made only for GTK port. If we agree to proceed with this approach, I will make changes to other build system as well.
Attachment 111220 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/InitializeWebKit.cpp:34: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/InitializeWebKit.cpp:34: __attribute__ is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > This method uses shared library constructor logic to initialize threading (Thanks to Martin for the pointers). This will ensure that threading gets initialized in all code paths. As per the GCC documentation http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html, this constructor is guaranteed to get called prior to calling application main function. What I meant by my comment was that we decided this approach was dangerous, so we didn't use it.
I'm not sure we need this, WebContext already initializes threading stuff.
(In reply to comment #9) > I'm not sure we need this, WebContext already initializes threading stuff. This issue is that WebPrerfences is independent of WebContext. If you use one before creating a WebContext, you'll see assertion failures, because threading is not initialized.
(In reply to comment #10) > (In reply to comment #9) > > I'm not sure we need this, WebContext already initializes threading stuff. > > This issue is that WebPrerfences is independent of WebContext. If you use one before creating a WebContext, you'll see assertion failures, because threading is not initialized. In that case we could initialize threading when creating WebPreferences object.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > I'm not sure we need this, WebContext already initializes threading stuff. > > > > This issue is that WebPrerfences is independent of WebContext. If you use one before creating a WebContext, you'll see assertion failures, because threading is not initialized. > > In that case we could initialize threading when creating WebPreferences object. WKPreference may not be the only object which has this issue (https://bugs.webkit.org/show_bug.cgi?id=70127#c4). Shared library constructor would ensure that threading gets initialized in all code path. However, since this seem to be dangerous, I think right fix for this is to identify all such objects and initialize threading while creating those objects.
Hi Darin, There are 2 approaches to fix this bug, a). Use shared library constructor approach (https://bugs.webkit.org/attachment.cgi?id=111220). This ensures that threading gets initialized in all code paths. However, as Martin pointed out, this seems to be somewhat risky (http://www.voyce.com/index.php/2009/12/03/dont-do-anything-in-dllmain-please/) b). Identify and initialize threading in all the entry points in UI Process (Which in turn doesn't launch WebProcess). However, such API additions in future should also do the similar setup. Which approach do you think is preferable? Please share your opinion.
I think approach (b) is the way to go. Since this is new API we do have some flexibility in making rules about how they need to be called; we don’t have to allow every public function call as a potentially first one.
So, I guess we could apply second attachment for now.
(In reply to comment #15) > So, I guess we could apply second attachment for now. Second patch has just the changes in WKPreference object. We may have to do similar changes in few other objects as well (https://bugs.webkit.org/show_bug.cgi?id=70127#c4). I am back from vacation, I should be able to submit the patch for this soon!
Created attachment 114060 [details] InitializeThreading Changes included in this patch are, a). Initialization of threading as per the approach (b) mentioned in https://bugs.webkit.org/show_bug.cgi?id=70127#c13. All the entry points in UIProcess and Shared API's are verified and threading initialization is done in those API's, whose arguments are of types which do NOT initialize threading. b). Added new files for compilation in all webkit2 ports. c). Rebase'ed to latest WebKit trunk.
Comment on attachment 114060 [details] InitializeThreading Attachment 114060 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10378121
Created attachment 114232 [details] Patch
(In reply to comment #19) > Created an attachment (id=114232) [details] > Patch EWS need to run a full build to get rid of linker errors reported earlier.
As Darin suggested we have the flexibility to define how the API should be used (of course not too restrictively). So, why don't we just require the user to create a context before doing anything else? Could you mention a specific use case where this is an issue?
(In reply to comment #21) > As Darin suggested we have the flexibility to define how the API should be used (of course not too restrictively). So, why don't we just require the user to create a context before doing anything else? Could you mention a specific use case where this is an issue? Creating context before doing anything else would work, but context created will be of no use in some use cases. For example, this use case might fail, WKPreferenceRef pref = WKPreferencesCreate(); bool enabled = WKPreferencesGetJavaScriptEnabled(WKPreferencesRef preferences); // -- Might fail here! To fix this, we may have to change the sequence of API usage to, WKContextRef context = WKContextCreate(); WKPreferenceRef pref = WKPreferencesCreate(); bool enabled = WKPreferencesGetJavaScriptEnabled(WKPreferencesRef preferences); // context go un-used here! Initializing threading in most of the entry paths would definitely makes the API usage a bit more friendly and less restrictive.
Comment on attachment 114232 [details] Patch Attachment 114232 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10376408
(In reply to comment #23) > (From update of attachment 114232 [details]) > Attachment 114232 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/10376408 As mentioned earlier, I think both gtk and win ews need to make a full build to get rid of these errors.
Comment on attachment 114232 [details] Patch Hmm, all this duplicated code makes me wonder if we could just do the initialization in the APIObject constructor instead. Would that work?
Created attachment 118387 [details] Initialize threading Incorporated review comments
Manually committed r102346: http://trac.webkit.org/changeset/102346