As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-23 16:26 PDT by Olli Pettay [:smaug] (review queue closed until backlog cleared)
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 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 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 User image 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 User image :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 User image 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 User image :Ehsan Akhgari 2010-03-15 15:25:22 PDT
Created attachment 432675 [details] [diff] [review]
Patch (v1)

Something like this?
Comment 5 User image 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 User image :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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-15 15:58:53 PDT
Ah, right! Well, carry on then :)
Comment 9 User image 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 User image 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 User image :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.