1445794 - Ensure AccessibleCaretEventHub's scroll and reflow callbacks don't flush

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

That breaks invariants.

Comment 1

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Mats, 

I'm thinking about adding nsAutoScriptBlocker to all the public entry points in AccessibleCaretEventHub [1] like you suggested in bug 1246918 Comment 26 so that no arbitary script can delete frames, and we can remove a lot of MOZ_CAN_RUN_SCRIPT annotation.

We might still need flush layout (at least styles) after dragging carets or selecting a word because AccessibleCaretManager needs to call Selection::Stringify [2] which flushes frames. I can invesigate whether there's any correctness issue if we don't flush in reflow or scroll callbacks. 

Does that sound like a good plan? 

[1] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/layout/base/AccessibleCaretEventHub.h#74,78,84-85,87-88,92,94,96,103-105
[2] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/layout/base/AccessibleCaretManager.cpp#1422
[3] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/dom/base/Selection.cpp#420

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(hide comment 2 because it contains typos and incorrect links)

Mats, 

I'm thinking about adding nsAutoScriptBlocker to all the public entry points in AccessibleCaretEventHub [1] as you suggested in bug 1246918 Comment 26 so that no arbitrary script can delete frames, and we can remove a lot of MOZ_CAN_RUN_SCRIPT annotation.

We might still need flush layout (at least styles) after dragging carets or selecting a word because AccessibleCaretManager needs to call Selection::Stringify [2] which flushes frames [3]. I can investigate whether there's any correctness issue if we don't flush in reflow or scroll callbacks. 

Does that sound like a good plan? 

[1] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/layout/base/AccessibleCaretEventHub.h#74,78,84-85,87-88,92,94,96,103-105
[2] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/layout/base/AccessibleCaretManager.cpp#1422
[3] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/dom/base/Selection.cpp#420

Comment 4

[Mats Palmgren (inactive)]

I'm not sure if that helps with the underlying issue, which is
that *the callers* can't handle destruction of various Layout data.
So when the On* methods return that code crashes.

So perhaps a better solution is to make sure that can't happen by
making the On* methods post a script runner to do its work later?

Picking a random example, AccessibleCaretManager::OnSelectionChanged,
would do something like:
  if (aSel->Type() != PRIMARY) return;  // or whatever
  AddScriptRunner(NewRunnableMethod(this, DoOnSelectionChanged))
and then:
DoOnSelectionChanged(...) {
  if (IsTerminated())
    return;
  nsAutoScriptBlocker scriptBlocker;
  if (!FlushLayout())
    return;
  ... rest of OnSelectionChanged code ...
}

I still think we need MOZ_CAN_RUN_SCRIPT on the DoOn* methods since
nsAutoScriptBlocker may run script when it exits the scope, but any
On* methods that just posts a script runner do not need that
annotation.

Would that work?

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Yep, I think moving the methods that flush layout to a runnable should work. I'll write patches based on the idea.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1445794 Part 1 - Preemptively fix that carets are not updated if non-default hints are used.

Without this change, for example,
UpdateCarets(UpdateCaretsHint::DispatchNoEvent) won't update carets. We don't
have a wrong use case yet, but it's good to fix that beforehand.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1445794 Part 2 - Make AccessibleCaretManager a refcount object.

We need AccessibleCaretManager to be a refcount object to ensure the validity
when using NewRunnableMethod() to construct runnables.

Depends on D15535

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1445794 Part 2 - Disallow flushing layout in reflow and scroll related callbacks.

These scrolling and reflow callbacks call UpdateCarets(), which need to
flush layout to ensure the AccessibleCaret's position is correct.
However, it's possible that PresShell and frame tree are destroyed after
doing so. To prevent this from affecting other callbacks executing after
the layout is flushed, we move carets callbacks to runnables.

Depends on D15536

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1445794 Part 4 - Prevent entering FlushLayout recursively.

OnReflow() can still call FlushLayout() that recursively enters
OnReflow() again, so we make FlushLayout() return early if it's already
flushing layout.

Depends on D15537

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34151b0c56e9
Part 1 - Preemptively fix that carets are not updated if non-default hints are used. r=emilio
https://hg.mozilla.org/integration/autoland/rev/267a452439f5
Part 2 - Disallow flushing layout in reflow and scroll related callbacks. r=emilio

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/34151b0c56e9
https://hg.mozilla.org/mozilla-central/rev/267a452439f5

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This bug is fixed. Clear the NI.