nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: smaug, Assigned: Ehsan)

Tracking

Trunk
mozilla1.9.3a4
x86
All
Points:
---

Firefox Tracking Flags

(status1.9.2 .4-fixed, status1.9.1 .10-fixed)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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
Whiteboard: [sg:investigate]

Updated

7 years ago
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) {
  ...
}
Created attachment 432675 [details] [diff] [review]
Patch (v1)

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
Last Resolved: 7 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+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/89dc7bcedfc6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d360dfcc44b0
status1.9.1: --- → .10-fixed
status1.9.2: --- → .3-fixed
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.