Bug 198622 – Crash using WKHTTPCookieStore when you use WKWebView and UIWebView in the same app

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Crash using WKHTTPCookieStore when you use WKWebView and UIWebView in the same app

[Geoffrey Garen]

Created attachment 371524 [details]
Patch

[Tim Horton]

Comment on attachment 371524 [details]
Patch

Nice!

[WebKit Commit Bot]

Comment on attachment 371524 [details]
Patch

Clearing flags on attachment: 371524

Committed r246174: <https://trac.webkit.org/changeset/246174>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51503523>

[Darin Adler]

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?

[Tim Horton]

(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?

[Darin Adler]

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.

[Darin Adler]

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.

[Tim Horton]

(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.