290425 - Selection details leak

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[rbs]

Comment 1

[rbs]

Attachment: fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180772 [details] [diff] [review]
fix

r+sr=bzbarsky, but shouldn't we actually skip looking at some of the output of
LookUpSelection() if it returns error?	Separate bug fodder, I guess...

Comment 3

[rbs]

Yeah. There are several spots in that file where the return status of
LookUpSelection() is ignored... I assume this leads to null selection details
(and  useless operations on that).

Comment 4

[rbs]

Comment on attachment 180772 [details] [diff] [review]
fix

Asking approval for 1.8b2. The patch fixes a memory leak on a codepath that is
accessed frequently.

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 180772 [details] [diff] [review]
fix

a=asa

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I assume this leads to null selection details (and  useless operations on
> that).

Would that it were so!  See 
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#2639

Any particular call to LookUpSelection in that loop can fail, but that failure
will be ignored; all that will happen is there will be no details for that
selection type in the list; the previous ones will still be there...

Comment 7

[rbs]

I was referring to nsTextFrame. Either way, null details and/or returns failure
don't create issues at present, except perhaps useless operations (in
nsTextFrame) which are minor.

Checked in.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> don't create issues at present

I can see it leading to wrong rendering, in general.  Then again, it's not clear
to me what the best course of action is on OOM while trying to render.

Comment 9

[rbs]

s/issues/critical issues/... Yeah, there is very little room for manoeuvring.