Closed Bug 1152432 Opened 7 years ago Closed 3 years ago

SelectionCarets needs to deal with stuff being destroyed after Flush_Layout

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mats, Unassigned)

References

Details

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.
Mark this as WONTFIX because SelectionCarets are replaced by AccessibleCaret.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
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.
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
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.
You need to log in before you can comment on or make changes to this bug.