Summary: | [WPE][GTK] Cleanups to the certificate encoder | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||
Component: | New Bugs | Assignee: | Claudio Saavedra <csaavedra> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | mcatanzaro, webkit-bug-importer, zan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Claudio Saavedra
2018-10-24 01:01:29 PDT
Created attachment 353027 [details]
Patch
Comment on attachment 353027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353027&action=review > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:61 > GByteArray* certificateData = 0; > Vector<GByteArray*> certificatesDataList; Move `certificateData` into the `do { }` scope, limiting its use to that part of the code. `certificatesDataList` should IMO be a Vector<GRefPtr<GByteArray>>, immediately adopting the `certificateData` pointer and avoiding the out-of-the-blue adoptGRef() int the loop below. > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:80 > + for (int i = certificatesDataList.size(); i > 0; i--) { Use size_t for `i`. In the post-loop segment, use the prefixed decrement: `--i`. Created attachment 353028 [details]
Patch
Comment on attachment 353028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353028&action=review > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:63 > + GByteArray* certificateData = 0; > g_object_get(G_OBJECT(certificate), "certificate", &certificateData, NULL); `nullptr` should be used in both lines. > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:80 > + GRefPtr<GByteArray> certificate = certificatesDataList[i - 1]; This takes an extra reference to the GByteArray pointer stored in the Vector. Use auto& to bind a C++ reference to the existing GRefPtr instead: `auto& certificate = ...` Committed r237377: <https://trac.webkit.org/changeset/237377> Comment on attachment 353028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353028&action=review > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:68 > certificatesDataList.append(certificateData); I think you need adoptGRef() here to avoid leaking it, right? >> Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:80 >> + GRefPtr<GByteArray> certificate = certificatesDataList[i - 1]; > > This takes an extra reference to the GByteArray pointer stored in the Vector. Use auto& to bind a C++ reference to the existing GRefPtr instead: > `auto& certificate = ...` It's less confusing if you bypass the GRefPtr (because you don't need to hold a ref here) and just go straight to the GByteArray: GByteArray* certificate = certificatesDataList[i - 1].get(); Oh I missed that you already committed (cq? was still set, try not to forget about that). Anyway, my second comment isn't important, but please check the first one to make sure you're not leaking. |