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

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: emilio, Assigned: TYLin)

Tracking

(Blocks 2 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

That breaks invariants.
Reporter

Updated

a year ago
Flags: needinfo?(emilio)
Assignee

Updated

a year ago
[Triage 2018/03/23 - P3]
Priority: -- → P3
Reporter

Updated

a year ago
See Also: → 1468140
Reporter

Updated

11 months ago
Blocks: 1470762
Reporter

Updated

8 months ago
Blocks: 1486521
Comment hidden (typo)
Assignee

Comment 3

7 months ago
(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
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?
Flags: needinfo?(mats)
Assignee

Comment 5

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

Updated

5 months ago
See Also: → 1515558
Assignee

Updated

5 months ago
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Assignee

Comment 6

5 months ago
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.
Assignee

Comment 7

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

Depends on D15535
Assignee

Comment 8

5 months ago
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
Assignee

Comment 9

5 months ago
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

Comment 10

5 months ago
Pushed by aethanyc@gmail.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34151b0c56e9
https://hg.mozilla.org/mozilla-central/rev/267a452439f5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee

Comment 12

5 months ago
This bug is fixed. Clear the NI.
Flags: needinfo?(emilio)
Assignee

Updated

5 months ago
No longer blocks: 1486521
Assignee

Updated

5 months ago
Duplicate of this bug: 1486521
Assignee

Updated

5 months ago
Blocks: 1486521
You need to log in before you can comment on or make changes to this bug.