484890 - nsEditingSession.cpp uses InitWithFuncCallback in a suspicious way

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditingSession.cpp#1061

What keeps docShell alive?

Comment 1

[Daniel Veditz [:dveditz]]

note: found as a follow-up to the bad pattern in sg:critical bug 484320

Comment 2

[(no longer active)]

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]

You can always use an nsWeakPtr. So


nsWeakPtr mFoo;


mFoo = do_GetWeakReference(docShell);


nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mFoo);
if (docShell) {
  ...
}

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Something like this?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[(no longer active)]

(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]

Ah, right! Well, carry on then :)

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/63a1cee09aa9

Comment 9

[Daniel Veditz [:dveditz]]

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 10

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/89dc7bcedfc6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d360dfcc44b0

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Is there anything for QA to reasonably do here to verify this fix for 1.9.1 and 1.9.2?

Comment 12

[(no longer active)]

(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.