Closed Bug 484890 Opened 11 years ago Closed 10 years ago

nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way

Categories

(Core :: Editor, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4
Tracking Status
status1.9.2 --- .4-fixed
status1.9.1 --- .10-fixed

People

(Reporter: smaug, Assigned: ehsan)

Details

(Whiteboard: [sg:critical?] [qa-noaction-191] [qa-noaction-192])

Attachments

(1 file)

note: found as a follow-up to the bad pattern in sg:critical bug 484320
Whiteboard: [sg:investigate]
Whiteboard: [sg:investigate] → [sg:critical?]
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) {
  ...
}
Attached patch Patch (v1)Splinter Review
Something like this?
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #432675 - Flags: review?(jonas)
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) ?
Attachment #432675 - Flags: review?(jonas) → review+
(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.
Ah, right! Well, carry on then :)
http://hg.mozilla.org/mozilla-central/rev/63a1cee09aa9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Attachment #432675 - Flags: approval1.9.2.3?
Attachment #432675 - Flags: approval1.9.1.10?
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
Attachment #432675 - Flags: approval1.9.2.3?
Attachment #432675 - Flags: approval1.9.2.3+
Attachment #432675 - Flags: approval1.9.1.10?
Attachment #432675 - Flags: approval1.9.1.10+
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.
Whiteboard: [sg:critical?] → [sg:critical?] [qa-noaction-191] [qa-noaction-192]
Group: core-security
You need to log in before you can comment on or make changes to this bug.