Bug 167776 – REGRESSION(r211486) [GTK] The MiniBrowser doesn't work anymore.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Alberto Lopez Perez]

After r211486 trying to open the WebKitGTK+ mini-browser timeouts/hangs.

Build WebKitGTK+ at 211616

1. $ Tools/Scripts/run-minibrowser --gtk https://www.google.com

... timeout .. nothing renders in the webview inside the minibrowser


2. Revert r211486 (only the changes on Source/JavaScriptCore/runtime/InitializeThreading.cpp)

3. Rebuild

4. $ Tools/Scripts/run-minibrowser --gtk https://www.google.com

.. now it works.

[Carlos Garcia Campos]

I haven't debugged it yet, but looking at the patch I think the problem is that the glib RunLoop implementation requires that WTF::initializeMainThread() is called before RunLoop::initializeMainRunLoop() This is because we need to know if we are the main thread run loop (and we can't compare to s_mainRunLoop because we are in the constructor. So, if we move the main loop initialization to JSC::initializeThreading() we should also do the initializeMainThread and remove those from InitializeWebKit2 and any other callers of JSC::initializeThreading() that also initialize main thread and main run loop

[Carlos Garcia Campos]

In some places we do:

JSC::initializeThreading();
WTF::initializeMainThread();

in others:

JSC::initializeThreading();
WTF::initializeMainThread();
RunLoop::initializeMainRunLoop();

and in others even:

WTF::initializeMainThread();
JSC::initializeThreading();

In iOS the web thread does this:

// WTF::initializeMainThread() needs to be called before JSC::initializeThreading() since the
// code invoked by the latter needs to know if it's running on the WebThread. See
// <rdar://problem/8502487>.

WTF::initializeMainThread();
WTF::initializeWebThread();
JSC::initializeThreading();

So, a quick solution to this bug could be doing this in InitializeWebKit2

WTF::initializeThreading();
WTF::initializeMainThread();
JSC::initializeThreading();

But I think we should check all the callers of all initializeThreading methods to make it consistent.

[Carlos Garcia Campos]

Created attachment 300506 [details]
Patch

[Carlos Garcia Campos]

*** Bug 167735 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Comment on attachment 300506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300506&action=review

r=me with the future refactoring proposal.

> Source/WebKit2/Shared/WebKit2Initialize.cpp:56
> +    JSC::initializeThreading();

It's strange to me that we need to care these ordering.
Why not calling RunLoop::initializeMainRunLoop() in WTF::initializeMainThread?
And why not calling WTF::initializeThreading and WTF::initializeMainThread in JSC::initializeThreading?
By using std::call_once, we can avoid the case that these functions are called more than once.
And after such a refactoring, I think just calling JSC::initializeThreading() here is OK.

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 300506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300506&action=review
> 
> r=me with the future refactoring proposal.
> 
> > Source/WebKit2/Shared/WebKit2Initialize.cpp:56
> > +    JSC::initializeThreading();
> 
> It's strange to me that we need to care these ordering.
> Why not calling RunLoop::initializeMainRunLoop() in
> WTF::initializeMainThread?

That was my plan, but then I need to check all the callers, and this is serious, so better fix it soon.

> And why not calling WTF::initializeThreading and WTF::initializeMainThread
> in JSC::initializeThreading?
> By using std::call_once, we can avoid the case that these functions are
> called more than once.
> And after such a refactoring, I think just calling
> JSC::initializeThreading() here is OK.

Yes, there are many things quite fragile in all threading initializations. I'm going to review all of them and all the callers and try a new simpler approach.

[WebKit Commit Bot]

Comment on attachment 300506 [details]
Patch

Clearing flags on attachment: 300506

Committed r211629: <http://trac.webkit.org/changeset/211629>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> > And why not calling WTF::initializeThreading and WTF::initializeMainThread
> > in JSC::initializeThreading?
> > By using std::call_once, we can avoid the case that these functions are
> > called more than once.
> > And after such a refactoring, I think just calling
> > JSC::initializeThreading() here is OK.
> 
> Yes, there are many things quite fragile in all threading initializations.
> I'm going to review all of them and all the callers and try a new simpler
> approach.

Yeah, we are now running into an issue on that iOS WebThread initialization path. Due to the order here we may end up doing RunLoop initialization on the WebThread instead of the main thread!

I think my original change is incorrect. I will rework it but ensure that we get the expected order of initialization here.

Follow on bug 167828.

[Joseph Pecoraro]

I rolled the original change out (and this follow-up since it depended on it) in:
https://trac.webkit.org/changeset/211666