Bug 198622 - Crash using WKHTTPCookieStore when you use WKWebView and UIWebView in the same app
Summary: Crash using WKHTTPCookieStore when you use WKWebView and UIWebView in the sam...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-06 13:43 PDT by Geoffrey Garen
Modified: 2019-06-06 17:29 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.63 KB, patch)
2019-06-06 13:48 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2019-06-06 13:43:20 PDT
Crash using WKHTTPCookieStore when you use WKWebView and UIWebView in the same app
Comment 1 Geoffrey Garen 2019-06-06 13:48:16 PDT
Created attachment 371524 [details]
Patch
Comment 2 Tim Horton 2019-06-06 15:50:57 PDT
Comment on attachment 371524 [details]
Patch

Nice!
Comment 3 WebKit Commit Bot 2019-06-06 16:00:12 PDT
Comment on attachment 371524 [details]
Patch

Clearing flags on attachment: 371524

Committed r246174: <https://trac.webkit.org/changeset/246174>
Comment 4 WebKit Commit Bot 2019-06-06 16:00:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-06-06 16:01:51 PDT
<rdar://problem/51503523>
Comment 6 Darin Adler 2019-06-06 16:52:18 PDT
Comment on attachment 371524 [details]
Patch

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

> Source/WebKit/ChangeLog:20
> +        (API::HTTPCookieStore::deleteCookie): Avoid using callOnMainThread
> +        becuase it is prohibited in the UI process.

Basic question: Does callOnMainThread already assert if it is called in the UI process?
Expert question: Is there a way to catch such problems at compile time instead of runtime?
Comment 7 Tim Horton 2019-06-06 17:01:00 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 371524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371524&action=review
> 
> > Source/WebKit/ChangeLog:20
> > +        (API::HTTPCookieStore::deleteCookie): Avoid using callOnMainThread
> > +        becuase it is prohibited in the UI process.
> 
> Basic question: Does callOnMainThread already assert if it is called in the
> UI process?

I don't think it does. We have the same problem with WebCore::Timer and I think the process to getting an assertion has been ... fraught. Mostly because it's OK (and necessary) for *legacy* WebKit to use it in the UI process, and telling the difference is hard. Dave Kilzer was working on this a while back.

> Expert question: Is there a way to catch such problems at compile time
> instead of runtime?
Comment 8 Darin Adler 2019-06-06 17:14:09 PDT
Comment on attachment 371524 [details]
Patch

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

>>> Source/WebKit/ChangeLog:20
>>> +        becuase it is prohibited in the UI process.
>> 
>> Basic question: Does callOnMainThread already assert if it is called in the UI process?
>> Expert question: Is there a way to catch such problems at compile time instead of runtime?
> 
> I don't think it does. We have the same problem with WebCore::Timer and I think the process to getting an assertion has been ... fraught. Mostly because it's OK (and necessary) for *legacy* WebKit to use it in the UI process, and telling the difference is hard. Dave Kilzer was working on this a while back.

Well, maybe some kind of compile time (or I guess style checker time?) checking would be easier for at least obvious cases like these, since WebKit and LegacyWebKit are in different source directories.
Comment 9 Darin Adler 2019-06-06 17:16:03 PDT
Comment on attachment 371524 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:20
>>>> +        becuase it is prohibited in the UI process.
>>> 
>>> Basic question: Does callOnMainThread already assert if it is called in the UI process?
>>> Expert question: Is there a way to catch such problems at compile time instead of runtime?
>> 
>> I don't think it does. We have the same problem with WebCore::Timer and I think the process to getting an assertion has been ... fraught. Mostly because it's OK (and necessary) for *legacy* WebKit to use it in the UI process, and telling the difference is hard. Dave Kilzer was working on this a while back.
> 
> Well, maybe some kind of compile time (or I guess style checker time?) checking would be easier for at least obvious cases like these, since WebKit and LegacyWebKit are in different source directories.

Another idea: Add a runtime check that only works in processes that haven’t yet invoked Legacy WebKit at all.
Bonus round: Make processes that aren’t apps explicitly using UIWebView not invoke Legacy WebKit at all.
Comment 10 Tim Horton 2019-06-06 17:29:35 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 371524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371524&action=review
> 
> >>>> Source/WebKit/ChangeLog:20
> >>>> +        becuase it is prohibited in the UI process.
> >>> 
> >>> Basic question: Does callOnMainThread already assert if it is called in the UI process?
> >>> Expert question: Is there a way to catch such problems at compile time instead of runtime?
> >> 
> >> I don't think it does. We have the same problem with WebCore::Timer and I think the process to getting an assertion has been ... fraught. Mostly because it's OK (and necessary) for *legacy* WebKit to use it in the UI process, and telling the difference is hard. Dave Kilzer was working on this a while back.
> > 
> > Well, maybe some kind of compile time (or I guess style checker time?) checking would be easier for at least obvious cases like these, since WebKit and LegacyWebKit are in different source directories.

That is true!

> Another idea: Add a runtime check that only works in processes that haven’t
> yet invoked Legacy WebKit at all.
> Bonus round: Make processes that aren’t apps explicitly using UIWebView not
> invoke Legacy WebKit at all.

See https://bugs.webkit.org/show_bug.cgi?id=185330 for some of the previous fun, but yes, I vaguely think Dave was doing some of these things. Just haven't gotten there yet.