393765 - Editor makes bogus assumptions about frame offsets

Status: NEW

(Core :: DOM: Editor, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

nsEditor::EndPlaceHolderTransaction has this comment:

   // By making the assumption that no reflow happens during the calls
   // to EndUpdateViewBatch and ScrollSelectionIntoView, we are able to
   // allow the selection to cache a frame offset which is used by the
   // caret drawing code.

Unfortunately, EndUpdateViewBatch can most certainly trigger reflow.  So this comment is just bogus.  Which makes me very suspicious of the code...

Do we actually need this cache anymore?  We only use it for ScrollSelectionIntoView, right?

Comment 1

[Eli Friedman]

This stuff was never very perf-sensitive, as far as I know... I'd say blow away the caching code.  GetPointFromOffset isn't so expensive that it's worth the trouble of caching the result.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The caching was added for bug 35296.  We should make sure to test that if we remove (which I whole heartedly support).

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What symptoms does this have?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Presumably incorrect caret placement in some cases.  At least I hope that's the worst that will happen.  If this cached frame stuff doesn't deal with frame destruction under EndUpdateViewBatch (which can easily happen), then we get the usual "accessing deleted frame" stuff.

Comment 5

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority and severity.

If you have reason to believe this is wrong, please write a comment and ni :jstutte.