Bug 199420 – ThreadSafeRefCounted<DestructionThread::Main> is not safe to use in the UIProcess

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

[Chris Dumez]

ThreadSafeRefCounted<DestructionThread::Main> is not safe to use in the UIProcess because its implementation relies on isMainThread() / callOnMainThread(). Those get confused about which thread is the main thread when an application uses both WK1 and WK2.

[Chris Dumez]

<rdar://problem/52289717>

[Ryosuke Niwa]

We can add an assertion to catch this?

[Chris Dumez]

Created attachment 373360 [details]
Patch

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #2)
> We can add an assertion to catch this?

I think it would be great yes, but I am not sure how to write it. I know similar issues came up in the past, did we come up with a solution to catch this?

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> (In reply to Ryosuke Niwa from comment #2)
> > We can add an assertion to catch this?
> 
> I think it would be great yes, but I am not sure how to write it. I know
> similar issues came up in the past, did we come up with a solution to catch
> this?

There is isInWebProcess() as a runtime check which is a bit cheesy. Ideally, it's something we initialize via XPC bootstrapping instead.

[Ryosuke Niwa]

Comment on attachment 373360 [details]
Patch

How about WebResourceLoadStatisticsStore?

[Ryosuke Niwa]

Also Storage in NetworkCacheStorage.

[Ryosuke Niwa]

Also, RegistrationDatabase and ServiceWorkerFetch::Client in WebCore uses it. Doesn't UI process use any of them?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 373360 [details]
> Patch
> 
> How about WebResourceLoadStatisticsStore?

Looks like it is in the NetworkProcess noawadays?

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> (In reply to Ryosuke Niwa from comment #2)
> > We can add an assertion to catch this?
> 
> I think it would be great yes, but I am not sure how to write it. I know
> similar issues came up in the past, did we come up with a solution to catch
> this?

See https://bugs.webkit.org/show_bug.cgi?id=185330

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #7)
> Also Storage in NetworkCacheStorage.

Again, this is in the NetworkProcess so not an issue.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #8)
> Also, RegistrationDatabase and ServiceWorkerFetch::Client in WebCore uses
> it. Doesn't UI process use any of them?

RegistrationDatabase is used only in the NetworkProcess so not a problem. ServiceWorkerFetch::Client is used in the Service Worker process I believe (So a WebProcess), thus not an issue either.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Ryosuke Niwa from comment #8)
> > Also, RegistrationDatabase and ServiceWorkerFetch::Client in WebCore uses
> > it. Doesn't UI process use any of them?
> 
> RegistrationDatabase is used only in the NetworkProcess so not a problem.
> ServiceWorkerFetch::Client is used in the Service Worker process I believe
> (So a WebProcess), thus not an issue either.

To be clear, classes that are used in the Network or WebContent processes, and only by WebKit2 code could use either DestructionThread::Main or DestructionThreadMainRunLoop. They would both be safe AFAIK. The only problematic ones are the ones used in the UIProcess, and I fixed the 2 classes I that were obviously used in the UIProcess.

[WebKit Commit Bot]

Comment on attachment 373360 [details]
Patch

Clearing flags on attachment: 373360

Committed r247078: <https://trac.webkit.org/changeset/247078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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