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> | ||||||
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
Dana Jansens
2012-05-16 10:50:14 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!
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. It seems like it should be public to me. What classes are you seeing that on? 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).
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 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() ?
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. Ah, I see. Having m_private always be non-NULL seems like the best of those options. Comment on attachment 142330 [details]
Patch
R=me then. Thanks for the explanation.
Comment on attachment 142330 [details]
Patch
Thanks!
Comment on attachment 142330 [details] Patch Clearing flags on attachment: 142330 Committed r117362: <http://trac.webkit.org/changeset/117362> All reviewed patches have been landed. Closing bug. |