Bug 70127 - [WebKit2] Initialize threading in UI process
Summary: [WebKit2] Initialize threading in UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nayan Kumar K
URL:
Keywords:
Depends on:
Blocks: 70135
  Show dependency treegraph
 
Reported: 2011-10-14 11:55 PDT by Nayan Kumar K
Modified: 2011-12-08 10:07 PST (History)
11 users (show)

See Also:


Attachments
Skip TestWebKitSettings test (1.54 KB, patch)
2011-10-14 12:04 PDT, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Initialize threading while creating WKPreference (2.49 KB, patch)
2011-10-14 12:36 PDT, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Initialize threading. (3.40 KB, patch)
2011-10-17 00:07 PDT, Nayan Kumar K
no flags Details | Formatted Diff | Diff
InitializeThreading (33.80 KB, patch)
2011-11-08 07:26 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Patch (35.29 KB, patch)
2011-11-09 03:15 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Initialize threading (8.78 KB, patch)
2011-12-08 07:33 PST, Nayan Kumar K
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nayan Kumar K 2011-10-14 11:55:07 PDT
Currently creating WKPreference doesn't initialize the threading (JSC::initializeThreading() and WTF::initializeMainThread()). This needs to be fixed, as WKPreference might be called independently.
Comment 1 Nayan Kumar K 2011-10-14 12:04:43 PDT
Created attachment 111043 [details]
Skip TestWebKitSettings test
Comment 2 Nayan Kumar K 2011-10-14 12:36:04 PDT
(In reply to comment #1)
> Created an attachment (id=111043) [details]
> Skip TestWebKitSettings test

Sorry, by mistake attached a wrong patch to wrong bug!!
Comment 3 Nayan Kumar K 2011-10-14 12:36:52 PDT
Created attachment 111053 [details]
Initialize threading while creating WKPreference
Comment 4 Darin Adler 2011-10-14 14:34:59 PDT
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.
Comment 5 Martin Robinson 2011-10-14 14:40:18 PDT
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.
Comment 6 Nayan Kumar K 2011-10-17 00:07:58 PDT
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.
Comment 7 WebKit Review Bot 2011-10-17 00:09:33 PDT
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.
Comment 8 Martin Robinson 2011-10-17 00:15:03 PDT
(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.
Comment 9 Carlos Garcia Campos 2011-10-17 00:25:22 PDT
I'm not sure we need this, WebContext already initializes threading stuff.
Comment 10 Martin Robinson 2011-10-17 00:30:32 PDT
(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.
Comment 11 Carlos Garcia Campos 2011-10-17 00:35:38 PDT
(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.
Comment 12 Nayan Kumar K 2011-10-17 00:46:09 PDT
(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.
Comment 13 Nayan Kumar K 2011-10-17 10:10:57 PDT
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.
Comment 14 Darin Adler 2011-10-17 10:21:02 PDT
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.
Comment 15 Carlos Garcia Campos 2011-11-02 07:24:50 PDT
So, I guess we could apply second attachment for now.
Comment 16 Nayan Kumar K 2011-11-03 05:44:18 PDT
(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!
Comment 17 Nayan Kumar K 2011-11-08 07:26:47 PST
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 18 Gustavo Noronha (kov) 2011-11-08 09:55:10 PST
Comment on attachment 114060 [details]
InitializeThreading

Attachment 114060 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10378121
Comment 19 Nayan Kumar K 2011-11-09 03:15:56 PST
Created attachment 114232 [details]
Patch
Comment 20 Nayan Kumar K 2011-11-09 03:17:57 PST
(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.
Comment 21 Balazs Kelemen 2011-11-09 04:32:02 PST
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?
Comment 22 Nayan Kumar K 2011-11-09 05:08:35 PST
(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 23 Collabora GTK+ EWS bot 2011-11-09 05:49:53 PST
Comment on attachment 114232 [details]
Patch

Attachment 114232 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10376408
Comment 24 Nayan Kumar K 2011-11-11 11:02:26 PST
(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 25 Anders Carlsson 2011-12-01 13:22:41 PST
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?
Comment 26 Nayan Kumar K 2011-12-08 07:33:17 PST
Created attachment 118387 [details]
Initialize threading

Incorporated review comments
Comment 27 Nayan Kumar K 2011-12-08 10:06:56 PST
Manually committed r102346: http://trac.webkit.org/changeset/102346