Closed
Bug 1131463
Opened 9 years ago
Closed 9 years ago
Make AtomicRefCountedWIthFinalize report problems if our current assumptions start failing
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: milan, Assigned: milan)
Details
Attachments
(1 file)
See the fix to bug 994903 and discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=994903#c16.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → milan
OS: Mac OS X → All
Assignee | ||
Comment 1•9 years ago
|
||
To summarize, the patch in bug 994903 fixes the problem described as such: 1. Thread 1 calls Release(), with the reference count at 2. We enter the function and do enough work to decide that we are the second last holder and that we should call the recycle callback function, so we go into the "else if (1 == currCount && mRecycleCallback)" branch, but then yield to another thread. 2. At that point, we swap to Thread 2, which also calls Release(), now as the last reference holder. We go into the "if (0 == currCount)" branch, delete "this" after calling Finalize() and exit. 3. We now go back to Thread 1, which tries to call a function whose pointer is in mRecycleCallback, except that is now on a deleted object and we get in trouble. The solution was to grab the function pointer in the local variable before we take either branch, so the fact that "this" is gone is irrelevant to Thread 1. Good so far. A couple of things I'd like to do just to clean up: * Add some more runtime checks just to make sure we report attempts to call Release() or AddRef() at bad times, like Release() when the object is not held. * Understand if this solution is OK long term - the recycle callback takes "this" as the argument, so if the callback actually uses it, then we're back with the original problem? Maybe bug 1032245? Probably not, but curious to find out.
Assignee | ||
Comment 2•9 years ago
|
||
I'm hoping I just don't understand what is going on, but with the current (possible mis)understanding, if the bug 994903 was being hit, we can have the last Release() happen, and the object go away, before we called the recycle callback.
Assignee | ||
Comment 3•9 years ago
|
||
Right - bug 994903 only fixes the case where we did not have a callback; the scenario where we do have the callback needs to be properly handled externally, but the reference holders, to make sure the ref count does not get to zero before we are done calling the recycle callback. As long as the users are doing the right thing, this should keep working.
Assignee | ||
Updated•9 years ago
|
Summary: Cleanup AtomicRefCountedWIthFinalize → Make AtomicRefCountedWIthFinalize report problems if our current assumptions start failing
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8566720 -
Flags: review?(sotaro.ikeda.g)
Comment 5•9 years ago
|
||
Comment on attachment 8566720 [details] [diff] [review] Add asserts to make sure we don't change the assumptions we make about reference counting AtomicRefCounterWithFinalize. r=sotaro Review of attachment 8566720 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/AtomicRefCountedWithFinalize.h @@ +54,5 @@ > RecycleCallback recycleCallback = mRecycleCallback; > int currCount = --mRefCount; > + if (currCount < 0) { > + gfxCriticalError() << "Invalid reference count release" << currCount; > + ++mRefCount; Is there a reason to increment here? Isn't detail::DEAD also "< 0", is it?
Attachment #8566720 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #5) > Comment on attachment 8566720 [details] [diff] [review] > Add asserts to make sure we don't change the assumptions we make about > reference counting AtomicRefCounterWithFinalize. r=sotaro > > Review of attachment 8566720 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/AtomicRefCountedWithFinalize.h > @@ +54,5 @@ > > RecycleCallback recycleCallback = mRecycleCallback; > > int currCount = --mRefCount; > > + if (currCount < 0) { > > + gfxCriticalError() << "Invalid reference count release" << currCount; > > + ++mRefCount; > > Is there a reason to increment here? Isn't detail::DEAD also "< 0", is it? Right - currCount < 0 means that we entered ::Release with mRefCount of 0 or detail::DEAD, and should not even try to do anything special. The idea was to leave mRefCount at the previous value if this happens. For example, it would be more confusing if we left it at -1 or DEAD-1. Thoughts?
Flags: needinfo?(milan)
Comment 8•9 years ago
|
||
Comment on attachment 8566720 [details] [diff] [review] Add asserts to make sure we don't change the assumptions we make about reference counting AtomicRefCounterWithFinalize. r=sotaro Review of attachment 8566720 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, it make sense.
Attachment #8566720 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab55f7fa2156
Keywords: checkin-needed
Comment 10•9 years ago
|
||
In the future, posting a rebased patch would be nice (especially since you did the work already for that Try push).
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d790f81f9f24
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d790f81f9f24
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•