Bug 167953 – Initialize the main RunLoop in iOS WebKitLegacy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Initialize the main RunLoop in iOS WebKitLegacy in WebKitInitialize.

Most API entry points initialize the main runloop on mac, but iOS ignores them. Do it here along with the other initialization.

[Joseph Pecoraro]

While this is under WebKitInitialize it is not inside it. Retitling.

[Joseph Pecoraro]

Created attachment 300843 [details]
[PATCH] Proposed Fix

[Alexey Proskuryakov]

Comment on attachment 300843 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/platform/ios/wak/WebCoreThread.mm:655
> +    RunLoop::initializeMainRunLoop();

Will RunLoop be confused because its initialization happens on the web thread, not the main one?

[Joseph Pecoraro]

Comment on attachment 300843 [details]
[PATCH] Proposed Fix

I believe you are correct. What concerns me is that there is an ASSERT(pthread_main_np()) that I added that doesn't fire, so now I'm rather confused.

[Joseph Pecoraro]

I found my problem. I added the ASSERT in WTF and built WTF and WebCore but didn't build JavaScriptCore. So my assertion didn't actually materialize. Thanks for the careful review

[Joseph Pecoraro]

Created attachment 300851 [details]
[PATCH] Proposed Fix

[Alexey Proskuryakov]

Comment on attachment 300851 [details]
[PATCH] Proposed Fix

r=me, however I have another thought about this. Since apparently nothing uses the runloop with iOS WK1, wouldn't it be better to leave it uninitialized for a clear failure to occur if someone tries to use it?

As we initialize it to the actual main thread, it may become more likely that WebCore code will schedule something to run on this thread, which is a bad bad idea.

[Alexey Proskuryakov]

I suggest double checking with Anders about what he thinks.

[Joseph Pecoraro]

(In reply to comment #7)
> Comment on attachment 300851 [details]
> [PATCH] Proposed Fix
> 
> r=me, however I have another thought about this. Since apparently nothing
> uses the runloop with iOS WK1, wouldn't it be better to leave it
> uninitialized for a clear failure to occur if someone tries to use it?
> 
> As we initialize it to the actual main thread, it may become more likely
> that WebCore code will schedule something to run on this thread, which is a
> bad bad idea.

There are already a few places in WebCore that use RunLoop::main, but as you point out in iOS WebKit1 they may want to be using the WebThread.

[Alexey Proskuryakov]

Maybe we should assert when trying to use an uninitialized RunLoop instead.

[Joseph Pecoraro]

> There are already a few places in WebCore that use RunLoop::main, but as you
> point out in iOS WebKit1 they may want to be using the WebThread.

Filed Bug 168008.

[Joseph Pecoraro]

(In reply to comment #10)
> Maybe we should assert when trying to use an uninitialized RunLoop instead.

That is the assert that is already firing in a few TestAPI Tests in iso-simulator already.

[WebKit Commit Bot]

Comment on attachment 300851 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 300851

Committed r211890: <http://trac.webkit.org/changeset/211890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> > Maybe we should assert when trying to use an uninitialized RunLoop instead.

> That is the assert that is already firing in a few TestAPI Tests in iso-simulator already.

I'm confused. Does this landed change mean that it won't fire any more?

[Joseph Pecoraro]

(In reply to comment #15)
> > > Maybe we should assert when trying to use an uninitialized RunLoop instead.
> 
> > That is the assert that is already firing in a few TestAPI Tests in iso-simulator already.
> 
> I'm confused. Does this landed change mean that it won't fire any more?

Correct this fixes those assertions by ensuring we initialize on iOS.