Closed Bug 1445794 Opened 6 years ago Closed 5 years ago

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


(Core :: Layout, enhancement, P3)




Tracking Status
firefox66 --- fixed


(Reporter: emilio, Assigned: TYLin)


(Blocks 1 open bug)



(2 files, 2 obsolete files)

That breaks invariants.
Flags: needinfo?(emilio)
[Triage 2018/03/23 - P3]
Priority: -- → P3
Blocks: 1470762
Blocks: 1486521
(hide comment 2 because it contains typos and incorrect links)


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? 

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())
  nsAutoScriptBlocker scriptBlocker;
  if (!FlushLayout())
  ... 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

Would that work?
Flags: needinfo?(mats)
Yep, I think moving the methods that flush layout to a runnable should work. I'll write patches based on the idea.
See Also: → 1515558
Assignee: nobody → aethanyc
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.
We need AccessibleCaretManager to be a refcount object to ensure the validity
when using NewRunnableMethod() to construct runnables.

Depends on D15535
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
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
Blocks: 1515558
See Also: 1515558
Attachment #9033808 - Attachment description: Bug 1445794 Part 3 - Move callbacks that handle scrolling and reflow to runnables. → Bug 1445794 Part 2 - Disallow flushing layout in reflow and scroll related callbacks.
Attachment #9033807 - Attachment is obsolete: true
Attachment #9033809 - Attachment is obsolete: true
Pushed by
Part 1 - Preemptively fix that carets are not updated if non-default hints are used. r=emilio
Part 2 - Disallow flushing layout in reflow and scroll related callbacks. r=emilio
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
This bug is fixed. Clear the NI.
Flags: needinfo?(emilio)
No longer blocks: 1486521
Blocks: 1486521
Summary: Ensure AccessibleEventCaret's scroll and reflow callbacks don't flush → Ensure AccessibleCaretEventHub's scroll and reflow callbacks don't flush
You need to log in before you can comment on or make changes to this bug.