Try fixing crash at com.apple.WebKit.Networking: NetworkProcess::setSharedHTTPCookieStorage

[Sihui Liu]

<rdar://problem/41325767>

[Sihui Liu]

Created attachment 375064 [details]
Patch

[Chris Dumez]

Comment on attachment 375064 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:624
> +        swap(m_networkProcess, networkProcess);

Indentation problem?
Also, why swap instead of a WTFMove()?

If you're right about the reason for the crash, wouldn't it mean that ensureNetworkProcess() is now likely to re-enter and re-construct m_networkProcess?

[Sihui Liu]

Created attachment 375092 [details]
Patch

[Chris Dumez]

Comment on attachment 375092 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:475
> +    if (!RunLoop::isMain()) {

Can we at lease add a ASSERT(RunLoop::isMain()); debug assertion?

> Source/WebKit/UIProcess/WebProcessPool.cpp:476
> +        callOnMainRunLoopAndWait([this] {

I am not convinced |this| is guaranteed to still be alive by the time you get to the main runloop. We likely this to protect/ref it in the lambda.

[Sihui Liu]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 375092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375092&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:475
> > +    if (!RunLoop::isMain()) {
> 
> Can we at lease add a ASSERT(RunLoop::isMain()); debug assertion?

Okay.

> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:476
> > +        callOnMainRunLoopAndWait([this] {
> 
> I am not convinced |this| is guaranteed to still be alive by the time you
> get to the main runloop. We likely this to protect/ref it in the lambda.

Will add.

[Sihui Liu]

Created attachment 375093 [details]
Patch

[Chris Dumez]

Comment on attachment 375093 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebProcessPool.cpp:476
> +    if (!RunLoop::isMain()) {

We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads.

[Geoffrey Garen]

Comment on attachment 375093 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:476
>> +    if (!RunLoop::isMain()) {
> 
> We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads.

Yes, let's please do that, with a link to a bug report. That way, future programmers have context, and will know under what conditions they can remove this / what they should test if they remove this.

[Sihui Liu]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 375093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375093&action=review
> 
> >> Source/WebKit/UIProcess/WebProcessPool.cpp:476
> >> +    if (!RunLoop::isMain()) {
> > 
> > We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads.
> 
> Yes, let's please do that, with a link to a bug report. That way, future
> programmers have context, and will know under what conditions they can
> remove this / what they should test if they remove this.

Sure, will add after the last bot turns green.

[Sihui Liu]

Created attachment 375127 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 375127 [details]
Patch for landing

Clearing flags on attachment: 375127

Committed r247933: <https://trac.webkit.org/changeset/247933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53687367>