Bug 176140

Summary: REGRESSION(r221226): [SOUP] libsoup-CRITICAL **: soup_cookies_to_cookie_header: assertion 'cookies != NULL' failed
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, buildbot, cgarcia, cturner, danw, gustavo, mcatanzaro
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch cgarcia: review+, cgarcia: commit-queue-

Description Michael Catanzaro 2017-08-30 18:10:05 PDT 
Charlie noticed this error in the buildbot logs:

libsoup-CRITICAL **: soup_cookies_to_cookie_header: assertion 'cookies != NULL' failed

I bet it's a regression from r221226.
Comment 1 Michael Catanzaro 2017-08-30 18:11:55 PDT 
Created attachment 319429 [details]
Patch
Comment 2 Michael Catanzaro 2017-08-30 18:13:05 PDT 
This is a speculative fix, but I'm fairly confident....
Comment 3 Carlos Garcia Campos 2017-08-30 23:15:42 PDT 
Comment on attachment 319429 [details]
Patch

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

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:92
> -    bool didAccessSecureCookies = false;
> +    if (!cookies)
> +        return { { }, false };
>  
> +    bool didAccessSecureCookies = false;

I think it would be better to do this check after the loop. If all cookies are secure and includeSecureCookies == IncludeSecureCookies::No, the list can be empty (nullptr) after the loop.
Comment 4 Charlie Turner 2017-08-31 05:22:58 PDT 
Michael, your speculative fix has worked for me, thanks!
Comment 5 Michael Catanzaro 2017-08-31 11:27:45 PDT 
(In reply to Carlos Garcia Campos from comment #3)
> I think it would be better to do this check after the loop. If all cookies
> are secure and includeSecureCookies == IncludeSecureCookies::No, the list
> can be empty (nullptr) after the loop.

Good catch.
Comment 6 Michael Catanzaro 2017-08-31 13:09:58 PDT 
Committed r221438: <http://trac.webkit.org/changeset/221438>