1152432 - SelectionCarets needs to deal with stuff being destroyed after Flush_Layout

Status: RESOLVED WONTFIX

(Core :: DOM: Selection, defect)

Description

[Mats Palmgren (inactive)]

http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=8df7f01875e8#442

The FlushPendingNotifications call on line 465 might lead to
Destroy() on mPresShell which calls Terminate on |this| and
then nulls out its reference which might delete |this|.
So we should proabably move this Flush to the start of the
method and make sure all callers of UpdateSelectionCarets()
holds a strong reference on the object they use for the
call (and bail out if mPresShell is null after that call).
We also need to make sure all callers of those methods
deals with it, recursively.

There is another Flush_Layout call here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=8df7f01875e8#1074
We need to fix this in the same way.

Also, callers of FlushPendingNotifications are required to
hold a strong ref on the shell for the duration of the call,
for example by holding a strong ref on the stack.

Comment 1

[Ting-Yu Lin [:TYLin] (UTC-8)]

Mark this as WONTFIX because SelectionCarets are replaced by AccessibleCaret.

Comment 2

[Mats Palmgren (inactive)]

AccessibleCaretManager::FlushLayout() has essentially the same problem though...
https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/base/AccessibleCaretManager.cpp#978
but I guess we'll fix that by removing the flush in bug 1445794.

Comment 3

[Ting-Yu Lin [:TYLin] (UTC-8)]

We had ensured all the entry points of AccessibleCaretEventHub' holds an extra strong reference in bug 1248847, and AccessibleCaretEventHub owns AccessibleCaretManager [1]. I think that implies AccessibleCaretManager's validity after layout flush, right?

[1] https://searchfox.org/mozilla-central/source/layout/base/AccessibleCaretEventHub.h#162

Comment 4

[Ting-Yu Lin [:TYLin] (UTC-8)]

The permanent link of [1] in comment 3 is https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/base/AccessibleCaretEventHub.h#162

Comment 5

[Mats Palmgren (inactive)]

True, but even if AccessibleCaretManager/AccessibleCaretEventHub is
robust against layout changes from the flush doesn't mean everything
else is.  It was used to trigger bug 1484559 and bug 1490561 just
recently for example.