Bug 42092 - [Chromium] Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp
Summary: [Chromium] Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-12 10:23 PDT by Chris Guillory
Modified: 2010-07-19 15:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch for adoptRef. (974 bytes, patch)
2010-07-12 11:04 PDT, Chris Guillory
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch. (1.05 KB, patch)
2010-07-12 13:59 PDT, Chris Guillory
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Proposed patch. (1.06 KB, patch)
2010-07-12 21:43 PDT, Chris Guillory
ctguil: commit-queue-
Details | Formatted Diff | Diff
Proposed patch. (1.77 KB, patch)
2010-07-12 23:06 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Guillory 2010-07-12 10:23:53 PDT
Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp
Comment 1 Chris Guillory 2010-07-12 11:04:08 PDT
Created attachment 61246 [details]
Patch for adoptRef.
Comment 2 Darin Adler 2010-07-12 12:58:04 PDT
Comment on attachment 61246 [details]
Patch for adoptRef.

> +    // We pass a reference to this object before it can be adopted.
> +    relaxAdoptionRequirement();
>      m_object->setWrapper(this);

This is not the correct way to fix this problem. Instead the call to setWrapper should be moved into the create function.
Comment 3 Chris Guillory 2010-07-12 13:59:17 PDT
Created attachment 61267 [details]
Proposed patch.

Moved adoptRef into create. Thanks for fast review.
Comment 4 Darin Adler 2010-07-12 14:03:07 PDT
Comment on attachment 61267 [details]
Proposed patch.

>      // FIXME: Remove resetting ref-count from AccessibilityObjectWrapper
> -    // and convert to use adoptRef.

Should add the period on the line you are keeping.

> -    return new WebAccessibilityCacheImpl::WeakHandle(object);
> +    RefPtr<WebAccessibilityCacheImpl::WeakHandle> weakHandle = adoptRef(new WebAccessibilityCacheImpl::WeakHandle(object));
> +    weakHandle->m_object->setWrapper(weakHandle.get());
> +    
> +    return weakHandle;

This should be "return weakHandle.release()" for best performance. Avoids one round of reference counting churn.

It looks like this change actually fixes a storage leak that was happening here. Or maybe not because of whatever "resetting ref-count from AccessibilityObjectWrapper" is.

I’ll say r=me but I think this is not completely right.
Comment 5 WebKit Commit Bot 2010-07-12 16:57:13 PDT
Comment on attachment 61267 [details]
Proposed patch.

Rejecting patch 61267 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 6 Chris Guillory 2010-07-12 21:43:58 PDT
Created attachment 61321 [details]
Proposed patch.

Added use of release(). Can someone commit this for me? The commit bot failed because there was no ChangeLog found. When I run prepareChangeLog it says "No changes found."
Comment 7 David Levin 2010-07-12 22:14:40 PDT
(In reply to comment #6)
> Created an attachment (id=61321) [details]
> Proposed patch.
> 
> Added use of release(). Can someone commit this for me? The commit bot failed because there was no ChangeLog found. When I run prepareChangeLog it says "No changes found."

If using git, you need to specify the --git-commit and give it an appropriate change like head~1.

If using svn, make sure to run prepareChangeLog from the root of your enlistment so it will see the change.

Regarding Darin's coment about this not being right, I looked over the code, and it does appear to be correct, but I couldn't figure out what that FIXME comment was about. (This does appear to be a leak that is getting fixed.)
Comment 8 Chris Guillory 2010-07-12 23:06:50 PDT
Created attachment 61331 [details]
Proposed patch.
Comment 9 Chris Guillory 2010-07-12 23:08:36 PDT
For some reason prepare-Changelog was having issues when I used --name to pass my name. It worked once I set the CHANGE_LOG_NAME and CHANGE_LOG_EMAIL_ADDRESS environment variables.
Comment 10 WebKit Commit Bot 2010-07-13 16:20:53 PDT
Comment on attachment 61331 [details]
Proposed patch.

Clearing flags on attachment: 61331

Committed r63258: <http://trac.webkit.org/changeset/63258>
Comment 11 WebKit Commit Bot 2010-07-13 16:20:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Guillory 2010-07-19 15:12:07 PDT
The FIXME comment is obsolete. It was done a while back.
http://trac.webkit.org/changeset/51402