Bug 86654 - [chromium] Clear the m_private pointer when destroying WebFilterOperations to avoid assert in WebPrivateOwnPtr
Summary: [chromium] Clear the m_private pointer when destroying WebFilterOperations to...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-16 10:50 PDT by Dana Jansens
Modified: 2012-05-16 16:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2012-05-16 10:51 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2012-05-16 13:20 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.