Last Comment Bug 484890 - nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way
: nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way
Status: RESOLVED FIXED
[sg:critical?] [qa-noaction-191] [qa-...
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla1.9.3a4
Assigned To: :Ehsan Akhgari
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-23 16:26 PDT by Olli Pettay [:smaug] (reviewing overload)
Modified: 2010-06-22 19:58 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4-fixed
.10-fixed


Attachments
Patch (v1) (2.13 KB, patch)
2010-03-15 15:25 PDT, :Ehsan Akhgari
jonas: review+
dveditz: approval1.9.2.4+
dveditz: approval1.9.1.10+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2009-03-23 16:26:38 PDT
http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditingSession.cpp#1061

What keeps docShell alive?
Comment 1 Daniel Veditz [:dveditz] 2009-03-23 17:48:00 PDT
note: found as a follow-up to the bad pattern in sg:critical bug 484320
Comment 2 :Ehsan Akhgari 2010-03-15 14:09:44 PDT
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?
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-15 14:28:57 PDT
You can always use an nsWeakPtr. So


nsWeakPtr mFoo;


mFoo = do_GetWeakReference(docShell);


nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mFoo);
if (docShell) {
  ...
}
Comment 4 :Ehsan Akhgari 2010-03-15 15:25:22 PDT
Created attachment 432675 [details] [diff] [review]
Patch (v1)

Something like this?
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-15 15:39:10 PDT
Comment on attachment 432675 [details] [diff] [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) ?
Comment 6 :Ehsan Akhgari 2010-03-15 15:51:49 PDT
(In reply to comment #5)
> (From update of attachment 432675 [details] [diff] [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.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-15 15:58:53 PDT
Ah, right! Well, carry on then :)
Comment 9 Daniel Veditz [:dveditz] 2010-03-19 14:18:53 PDT
Comment on attachment 432675 [details] [diff] [review]
Patch (v1)

Approved for 1.9.2.3 and 1.9.1.10, a=dveditz for release-drivers
Comment 11 Al Billings [:abillings] 2010-04-07 16:52:42 PDT
Is there anything for QA to reasonably do here to verify this fix for 1.9.1 and 1.9.2?
Comment 12 :Ehsan Akhgari 2010-04-07 17:22:26 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.