Bug 42092 – [Chromium] Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp

• Component: Accessibility
• Status: RESOLVED FIXED

[Chris Guillory]

Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp

[Chris Guillory]

Created attachment 61246 [details]
Patch for adoptRef.

[Darin Adler]

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.

[Chris Guillory]

Created attachment 61267 [details]
Proposed patch.

Moved adoptRef into create. Thanks for fast review.

[Darin Adler]

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.

[WebKit Commit Bot]

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

[Chris Guillory]

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."

[David Levin]

(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.)

[Chris Guillory]

Created attachment 61331 [details]
Proposed patch.

[Chris Guillory]

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.

[WebKit Commit Bot]

Comment on attachment 61331 [details]
Proposed patch.

Clearing flags on attachment: 61331

Committed r63258: <http://trac.webkit.org/changeset/63258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Guillory]

The FIXME comment is obsolete. It was done a while back.
http://trac.webkit.org/changeset/51402