Bug 200886 - [SOUP] NetworkProcessSoup does not initialize CacheOptions correctly
Summary: [SOUP] NetworkProcessSoup does not initialize CacheOptions correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-19 06:44 PDT by enometh
Modified: 2019-08-22 02:40 PDT (History)
7 users (show)

See Also:


Attachments
sketch (1.37 KB, patch)
2019-08-19 06:44 PDT, enometh
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2019-08-22 01:39 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2019-08-22 01:40 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description enometh 2019-08-19 06:44:28 PDT
Created attachment 376683 [details]
sketch

git commit 464bf0 http://svn.webkit.org/repository/webkit/trunk@247567
overlooked propagating the cacheOptions which are set in WebKit::NetworkProcess::platformInitializeNetworkProcess

The attached patch outlines a fix
Comment 1 enometh 2019-08-19 06:59:03 PDT
(The  attached patch unnecessarily marked the slot on NetworkProcess as protected . It shouldn't
 I hope this does not confuse the main issue.)
Comment 2 Claudio Saavedra 2019-08-22 00:59:45 PDT
Comment on attachment 376683 [details]
sketch

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

> Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:121
> +    cacheOptions.add(NetworkCache::CacheOption::RegisterNotify);

I'm confused by the original code here. I don't see this cacheOptions local variable used anywhere, only set here but never read. Is this correct at all or am I missing something?
Comment 3 Claudio Saavedra 2019-08-22 01:01:30 PDT
I think the code must have been wrong already before achristensen's patch. Here we should be setting m_cacheOptions, not a local variable.
Comment 4 Claudio Saavedra 2019-08-22 01:39:02 PDT
Created attachment 376996 [details]
Patch
Comment 5 Claudio Saavedra 2019-08-22 01:40:37 PDT
Created attachment 376997 [details]
Patch
Comment 6 Adrian Perez 2019-08-22 01:46:54 PDT
Comment on attachment 376997 [details]
Patch

r+, patch LGTM (reviewing informally)
Comment 7 WebKit Commit Bot 2019-08-22 02:40:32 PDT
Comment on attachment 376997 [details]
Patch

Clearing flags on attachment: 376997

Committed r248999: <https://trac.webkit.org/changeset/248999>
Comment 8 WebKit Commit Bot 2019-08-22 02:40:33 PDT
All reviewed patches have been landed.  Closing bug.