|Summary:||[chromium] Clear the m_private pointer when destroying WebFilterOperations to avoid assert in WebPrivateOwnPtr|
|Product:||WebKit||Reporter:||Dana Jansens <danakj>|
|Component:||New Bugs||Assignee:||Dana Jansens <danakj>|
|Severity:||Normal||CC:||abarth, cc-bugs, dglazkov, enne, fishd, jamesr, tkent+wkapi, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Dana Jansens 2012-05-16 10:50:14 PDT
[chromium] Clear the m_private pointer when destroying WebFilterOperations to avoid assert in WebPrivateOwnPtr
Comment 1 Dana Jansens 2012-05-16 10:51:29 PDT
Created attachment 142298 [details] Patch The ::reset() method is private on other WebClasses, but I can't imagine why it would be. So I've made it private here. If this is crazy please say so!
Comment 2 WebKit Review Bot 2012-05-16 10:55:36 PDT
Please wait for approval from firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com or firstname.lastname@example.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adrienne Walker 2012-05-16 12:30:51 PDT
It seems like it should be public to me. What classes are you seeing that on?
Comment 4 James Robinson 2012-05-16 12:35:21 PDT
Comment on attachment 142298 [details] Patch It's normally public since it makes sense for someone using a WebFilterOperations to want to get a fresh one without having to make a new object (or wrap it in a pointer or the like).
Comment 5 Dana Jansens 2012-05-16 13:20:19 PDT
Created attachment 142330 [details] Patch K made reset() public. Dropping clear() since it's redundant then. Adding private destroy() to clear the m_private pointer so that toFilterOperations() can remain const.
Comment 6 James Robinson 2012-05-16 15:24:13 PDT
Comment on attachment 142330 [details] Patch reset() in WebKit API normally puts you in a null state (and there's often a bool isNull() call to check it), so it's a bit odd to have initialize() construct something non-null. Maybe leave initialize() and have ~ call reset() ?
Comment 7 Dana Jansens 2012-05-16 15:42:19 PDT
I tried this at first, but the problem is toFilterOperations() is const and returns a reference, not a pointer. So if the m_private is null, then you can't call toFilterOperations() without a segfault, because you can't change m_private. Options: 1) Changing toFilterOperations() to non-const: breaks the WebLayer API which takes a const WebFilterOperations&. 2) Changing toFilterOperations() to return a pointer? 3) Make a static empty FilterOperations object to return when m_private is NULL? 4) Don't let m_private be NULL.
Comment 8 James Robinson 2012-05-16 15:44:18 PDT
Ah, I see. Having m_private always be non-NULL seems like the best of those options.
Comment 9 James Robinson 2012-05-16 15:44:42 PDT
Comment on attachment 142330 [details] Patch R=me then. Thanks for the explanation.
Comment 10 Dana Jansens 2012-05-16 15:46:04 PDT
Comment on attachment 142330 [details] Patch Thanks!
Comment 11 WebKit Review Bot 2012-05-16 16:34:31 PDT
Comment on attachment 142330 [details] Patch Clearing flags on attachment: 142330 Committed r117362: <http://trac.webkit.org/changeset/117362>
Comment 12 WebKit Review Bot 2012-05-16 16:34:36 PDT
All reviewed patches have been landed. Closing bug.