Bugzilla@Mozilla – Bug 484890
nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way
Last modified: 2010-06-22 19:58:14 PDT
Summon comment box
http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditingSession.cpp#1061 What keeps docShell alive?
note: found as a follow-up to the bad pattern in sg:critical bug 484320
So, we cancel the timer in the destructor here as well (http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditingSession.cpp#121), but I assume that it's not enough, because the docshell might have a different lifetime. Do we have any mechanism to detect whether a docshell pointer is valid? Something like weak frames?
You can always use an nsWeakPtr. So nsWeakPtr mFoo; mFoo = do_GetWeakReference(docShell); nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mFoo); if (docShell) { ... }
Created attachment 432675 [details] [review] Patch (v1) Something like this?
Comment on attachment 432675 [details] [review] Patch (v1) Looks correct. But I can't comment on if the destructor code makes this redundant or not. Though, do you even need the closure here? Can't you simply use mDocShell instead of static_cast<nsIWeakReference*> (aClosure) ?
(In reply to comment #5) > (From update of attachment 432675 [details] [review]) > Looks correct. But I can't comment on if the destructor code makes this > redundant or not. I tried to figure it out as well, but couldn't. I'm not sure if the lifetime of the docshell is always greater or equal to the lifetime of the editing session. But anyway this way we'll be safer, and there's no real perf loss for this patch. > Though, do you even need the closure here? Can't you simply use mDocShell > instead of static_cast<nsIWeakReference*> (aClosure) ? That's not possible, the callback timer is a static method.
Ah, right! Well, carry on then :)
http://hg.mozilla.org/mozilla-central/rev/63a1cee09aa9
Comment on attachment 432675 [details] [review] Patch (v1) Approved for 1.9.2.3 and 1.9.1.10, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/89dc7bcedfc6 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d360dfcc44b0
Is there anything for QA to reasonably do here to verify this fix for 1.9.1 and 1.9.2?
(In reply to comment #11) > Is there anything for QA to reasonably do here to verify this fix for 1.9.1 and > 1.9.2? Not really, we don't have a testcase which uses this possible flaw.