Bug 70127 – [WebKit2] Initialize threading in UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Nayan Kumar K]

Currently creating WKPreference doesn't initialize the threading (JSC::initializeThreading() and WTF::initializeMainThread()). This needs to be fixed, as WKPreference might be called independently.

[Nayan Kumar K]

Created attachment 111043 [details]
Skip TestWebKitSettings test

[Nayan Kumar K]

(In reply to comment #1)
> Created an attachment (id=111043) [details]
> Skip TestWebKitSettings test

Sorry, by mistake attached a wrong patch to wrong bug!!

[Nayan Kumar K]

Created attachment 111053 [details]
Initialize threading while creating WKPreference

[Darin Adler]

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.

[Martin Robinson]

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.

[Nayan Kumar K]

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.

[WebKit Review Bot]

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.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

I'm not sure we need this, WebContext already initializes threading stuff.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

(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.

[Nayan Kumar K]

(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.

[Nayan Kumar K]

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.

[Darin Adler]

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.

[Carlos Garcia Campos]

So, I guess we could apply second attachment for now.

[Nayan Kumar K]

(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!

[Nayan Kumar K]

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.

[Gustavo Noronha (kov)]

Comment on attachment 114060 [details]
InitializeThreading

Attachment 114060 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10378121

[Nayan Kumar K]

Created attachment 114232 [details]
Patch

[Nayan Kumar K]

(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.

[Balazs Kelemen]

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?

[Nayan Kumar K]

(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.

[Collabora GTK+ EWS bot]

Comment on attachment 114232 [details]
Patch

Attachment 114232 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10376408

[Nayan Kumar K]

(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.

[Anders Carlsson]

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?

[Nayan Kumar K]

Created attachment 118387 [details]
Initialize threading

Incorporated review comments

[Nayan Kumar K]

Manually committed r102346: http://trac.webkit.org/changeset/102346