Bug 86654

Summary: [chromium] Clear the m_private pointer when destroying WebFilterOperations to avoid assert in WebPrivateOwnPtr
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, dglazkov, enne, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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 abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.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.