Bug 173738 – isMainThread() assertions in CallbackMap are incorrectly failing on some iOS apps

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

[Chris Dumez]

isMainThread() assertions in CallbackMap are incorrectly failing on some iOS apps.

[Chris Dumez]

<rdar://problem/32923933>

[Chris Dumez]

Created attachment 313664 [details]
Patch

[Chris Dumez]

Committed r218724: <http://trac.webkit.org/changeset/218724>

[Daniel Bates]

We use WTF::isMainThread() all over the place in the code base, including throughout WebKit2. Do you plan to these to RunLoop::isMain()? Can we get rid of WTF::isMainThread()?

[Daniel Bates]

(In reply to Daniel Bates from comment #4)
> Do you plan to these to RunLoop::isMain()?

*Do you plan to update these to use RunLoop::isMain()?

[Chris Dumez]

(In reply to Daniel Bates from comment #5)
> (In reply to Daniel Bates from comment #4)
> > Do you plan to these to RunLoop::isMain()?
> 
> *Do you plan to update these to use RunLoop::isMain()?

Given the apps that were impacted and the implementation of isMainThread(), we suspect the issue has to do with mixing WebKit1 and WebKit2 in the UIProcess.

So yes, I think we should update assertions running in the UIProcess to use RunLoop::isMain() rather than isMainThread().

[Daniel Bates]

(In reply to Chris Dumez from comment #6)
> (In reply to Daniel Bates from comment #5)
> > (In reply to Daniel Bates from comment #4)
> > > Do you plan to these to RunLoop::isMain()?
> > 
> > *Do you plan to update these to use RunLoop::isMain()?
> 
> Given the apps that were impacted and the implementation of isMainThread(),
> we suspect the issue has to do with mixing WebKit1 and WebKit2 in the
> UIProcess.
> 
> So yes, I think we should update assertions running in the UIProcess to use
> RunLoop::isMain() rather than isMainThread().

"think"? Is RunLoop:isMain() the solution or not? If it is then please substitute RunLoop:isMain() in all UIProcess code that uses WTF::isMainThread(). If we cannot do such substitution mechanically then please explain how to interpret the second sentence of your ChangeLog message.

[Chris Dumez]

(In reply to Daniel Bates from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Daniel Bates from comment #5)
> > > (In reply to Daniel Bates from comment #4)
> > > > Do you plan to these to RunLoop::isMain()?
> > > 
> > > *Do you plan to update these to use RunLoop::isMain()?
> > 
> > Given the apps that were impacted and the implementation of isMainThread(),
> > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the
> > UIProcess.
> > 
> > So yes, I think we should update assertions running in the UIProcess to use
> > RunLoop::isMain() rather than isMainThread().
> 
> "think"? Is RunLoop:isMain() the solution or not? If it is then please
> substitute RunLoop:isMain() in all UIProcess code that uses
> WTF::isMainThread(). If we cannot do such substitution mechanically then
> please explain how to interpret the second sentence of your ChangeLog
> message.

What I said, upon local testing RunLoop::isMain() was returning the correct result but WTF::isMainThread() wasn't.

My only speculation is about why WTF::isMainThread() does not return the correct result. That part I do not know for sure. My priority was to unbreak those apps.

A mechanical change in WebKit2 sounds fine.

[Chris Dumez]

(In reply to Daniel Bates from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Daniel Bates from comment #5)
> > > (In reply to Daniel Bates from comment #4)
> > > > Do you plan to these to RunLoop::isMain()?
> > > 
> > > *Do you plan to update these to use RunLoop::isMain()?
> > 
> > Given the apps that were impacted and the implementation of isMainThread(),
> > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the
> > UIProcess.
> > 
> > So yes, I think we should update assertions running in the UIProcess to use
> > RunLoop::isMain() rather than isMainThread().
> 
> "think"? Is RunLoop:isMain() the solution or not? If it is then please
> substitute RunLoop:isMain() in all UIProcess code that uses
> WTF::isMainThread(). If we cannot do such substitution mechanically then
> please explain how to interpret the second sentence of your ChangeLog
> message.

FYI, you make it really hard for foreigners like me to talk to you if you're going to pick on a given wording like that.

[Daniel Bates]

(In reply to Chris Dumez from comment #9)
> (In reply to Daniel Bates from comment #7)
> > (In reply to Chris Dumez from comment #6)
> > > (In reply to Daniel Bates from comment #5)
> > > > (In reply to Daniel Bates from comment #4)
> > > > > Do you plan to these to RunLoop::isMain()?
> > > > 
> > > > *Do you plan to update these to use RunLoop::isMain()?
> > > 
> > > Given the apps that were impacted and the implementation of isMainThread(),
> > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the
> > > UIProcess.
> > > 
> > > So yes, I think we should update assertions running in the UIProcess to use
> > > RunLoop::isMain() rather than isMainThread().
> > 
> > "think"? Is RunLoop:isMain() the solution or not? If it is then please
> > substitute RunLoop:isMain() in all UIProcess code that uses
> > WTF::isMainThread(). If we cannot do such substitution mechanically then
> > please explain how to interpret the second sentence of your ChangeLog
> > message.
> 
> FYI, you make it really hard for foreigners like me to talk to you if you're
> going to pick on a given wording like that.

How do you suggest a person interpret your wording? I mean, the only available information that the general public has is what is written in the comments in this bug and in the ChangeLog/commit message. Maybe it would be better to discuss this over a different medium?

[Daniel Bates]

(In reply to Chris Dumez from comment #8)
> (In reply to Daniel Bates from comment #7)
> > (In reply to Chris Dumez from comment #6)
> > > (In reply to Daniel Bates from comment #5)
> > > > (In reply to Daniel Bates from comment #4)
> > > > > Do you plan to these to RunLoop::isMain()?
> > > > 
> > > > *Do you plan to update these to use RunLoop::isMain()?
> > > 
> > > Given the apps that were impacted and the implementation of isMainThread(),
> > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the
> > > UIProcess.
> > > 
> > > So yes, I think we should update assertions running in the UIProcess to use
> > > RunLoop::isMain() rather than isMainThread().
> > 
> > "think"? Is RunLoop:isMain() the solution or not? If it is then please
> > substitute RunLoop:isMain() in all UIProcess code that uses
> > WTF::isMainThread(). If we cannot do such substitution mechanically then
> > please explain how to interpret the second sentence of your ChangeLog
> > message.
> 
> What I said, upon local testing RunLoop::isMain() was returning the correct
> result but WTF::isMainThread() wasn't.
> 
> My only speculation is about why WTF::isMainThread() does not return the
> correct result. That part I do not know for sure. My priority was to unbreak
> those apps.
> 
> A mechanical change in WebKit2 sounds fine.

Filed bug #173745 to investigate WTF::isMainThread() and/or substitute RunLoop:isMain() for uses of WTF::isMainThread().

[Daniel Bates]

(In reply to Chris Dumez from comment #9)
> FYI, you make it really hard for foreigners like me to talk to you if you're
> going to pick on a given wording like that.

This has nothing to do with you being a foreigner. I just wanted to clarify whether using RunLoop:isMain() is the correct solution to fix this bug (as opposed to fixing WTF::isMainThread() or some other solution) as well as ascertain when can we expect to replace uses of WTF::isMainThread() with RunLoop:isMain() throughout our codebase assuming using RunLoop:isMain() is the correct solution. We now have bug #173745 tracking this clarification/discussion.