1246918 - AccessibleCaret disappears when swiping down the page

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

Steps to reproduce on Fennec:
1. Open an arbitrary webpage, and do not scroll or zoom the page yet.
2. Long press to select a word.
3. Swipe down on the screen to scroll the page down.

Expected result:
The carets should show again after the scroll stops.

Actual results:
The carets disappear. However it'll show when scrolling up.

The key to reproduce the bug is that the selection highlight does not change its position when swiping down since the page is at top.

Comment 1

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

Attachment: MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

Fennec enables sCaretsExtendedVisibility which uses
Appearance::NormalNotShown instead of Appearance::None to keep actionbar
shown during scrolling. This breaks selection mode update when the
positions of the carets are not changed after scrolling.

To fix this, we need to implement appearance recovering for selection
mode scrolling like we did for cursor mode in bug 1212732, and make
UpdateCaretsForSelectionMode() respects UpdateCaretsHint.

Review commit: https://reviewboard.mozilla.org/r/34183/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34183/

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8717466 [details]
MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

https://reviewboard.mozilla.org/r/34183/#review30885

Comment 3

[Mats Palmgren (inactive)]

https://reviewboard.mozilla.org/r/34183/#review30973

::: layout/base/AccessibleCaretManager.cpp:600
(Diff revision 1)
> +  FlushLayout();

The FlushLayout call here worries me a bit.  I suspect |this| might be deleted by it (which could lead to UAF later in this method).

Comment 4

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

https://reviewboard.mozilla.org/r/34183/#review30973

> The FlushLayout call here worries me a bit.  I suspect |this| might be deleted by it (which could lead to UAF later in this method).

PresShell keeps a RefPtr to AccessibleCaretEventHub, and AccessibleCaretEventHub keeps a UniquePtr to AccessibleCaretManager, which is |this| here. Thus PresShell should keep |this| alive unless the FlushLayout() call destorys PresShell. Personally, I haven't encounter that calling FlushLayout() in AccessibleCaretManager destorys the PresShell. Mats, do you feel it is possible?

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a957202eace4

Comment 6

[Mats Palmgren (inactive)]

FlushPendingNotifications can run script (unless you have a nsAutoScriptBlocker on the stack
here somewhere) which means pretty much anything can happen including destroying the presshell.
If you block script then it can only destroy frames and other layout/style related objects.

Comment 8

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

Attachment: MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

Fennec enables sCaretsExtendedVisibility which uses
Appearance::NormalNotShown instead of Appearance::None to keep actionbar
shown during scrolling. This breaks selection mode update when the
positions of the carets are not changed after scrolling.

To fix this, we need to implement appearance recovering for selection
mode scrolling like we did for cursor mode in bug 1212732, and make
UpdateCaretsForSelectionMode() respects UpdateCaretsHint.

Review commit: https://reviewboard.mozilla.org/r/34475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34475/

Comment 9

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

Attachment: MozReview Request: Bug 1246918 - Block script in FlushLayout(). r=mats

Per bug 1246918 comment 6, we should block script when flushing layout
to prevent PresShell being destroyed.

Review commit: https://reviewboard.mozilla.org/r/34477/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34477/

Comment 10

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

Comment on attachment 8718238 [details]
MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

I didn't change this patch, and had no idea why mozreview clear the r+. Carry r+ by roc in comment 2.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8718238 [details]
MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

https://reviewboard.mozilla.org/r/34475/#review31175

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/34477/#review31179

::: layout/base/AccessibleCaretManager.cpp:816
(Diff revision 1)
> +    nsAutoScriptBlocker scriptBlocker;

I don't think this will actually work. When the scriptBlocker goes away, the scripts will run so AccessibleCaretManager could still potentially be destroyed in this method.

Instead, where we call into AccessibleCaretEventHub methods that could reach FlushLayout, we need to hold a strong reference to the AccessibleCaretEventHub or the PresShell. Preferably the PresShell.

Comment 13

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

https://reviewboard.mozilla.org/r/34477/#review31179

> I don't think this will actually work. When the scriptBlocker goes away, the scripts will run so AccessibleCaretManager could still potentially be destroyed in this method.
> 
> Instead, where we call into AccessibleCaretEventHub methods that could reach FlushLayout, we need to hold a strong reference to the AccessibleCaretEventHub or the PresShell. Preferably the PresShell.

Ah. You're right.

Comment 14

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

Comment on attachment 8718238 [details]
MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34475/diff/1-2/

Comment 15

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

Attachment: MozReview Request: Bug 1246918 - Prevent FlushLayout() from destroying PresShell. r=roc

In the previous patch, FlushLayout() added in OnScrollEnd() might
destroy PresShell. To keep mPresShell valid in all the AccessibleCaret
operations including FlushLayout(), I add a pointer to hold PresShell in
all entry points to AccessibleCaretEventHub to keep mPresShell live.

Remove FlushLayout() in OnSelectionChanged() since it isn't needed
before calling UpdateCaret().

Review commit: https://reviewboard.mozilla.org/r/34511/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34511/

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8718316 [details]
MozReview Request: Bug 1246918 - Prevent FlushLayout() from destroying PresShell. r=roc

https://reviewboard.mozilla.org/r/34511/#review31259

::: layout/base/AccessibleCaretEventHub.cpp:478
(Diff revision 1)
> +  nsCOMPtr<nsIPresShell> presShell = mPresShell;

This is too late. We don't want AccessibleCaretEventHub to be destroyed while we're inside its method, that's dangerous. The caller(s) of this method (and the other methods) need to hold the presshell.

Comment 17

[Mats Palmgren (inactive)]

Note that holding a strong ref on the shell is NOT enough avoid AccessibleCaret*
from being deleted.  You can't prevent PresShell::Destroy() from being called
by holding a strong ref, it just prevents the shell from being deleted (deallocated).

You need both a strong ref AND checking shell->IsDestroying() after the flush
call, and after all calls to any method that flushes, transitively.

Alternatively, you can put a nsAutoScriptBlocker object on the stack at all entry
points into such a path.  With that, a strong ref on the shell is enough.

Comment 18

[Mats Palmgren (inactive)]

A third option could be to add a strong ref on the AccessibleCaretEventHub on the stack
at all entry points into paths that can lead to a flush.  That should prevent AccessibleCaret*
from being deleted (assuming they are ref counted).  But you probably also want to check
shell->IsDestroying() and abort what you're doing in that case in a number of places. 
(You shouldn't use layout objects or the shell itself after IsDestroying() is true.)

Comment 19

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

(In reply to Mats Palmgren (:mats) from comment #17)
> Alternatively, you can put a nsAutoScriptBlocker object on the stack at all
> entry points into such a path.  With that, a strong ref on the shell is enough.

Mats, could you elaborate more on why nsAutoScriptBlocker could prevent PresShell::Destroy() being called? It's not straightforward to me. It looks like in order to call PresShell::Destroy(), the script must be blocked in [1].

After looking closer, I think your third option in comment 17 is the most feasible solution because AccessibleCaretEventHub is terminated and deref in PresShell::Destroy(). We cannot hold a strong ref to PresShell to keep AccessibleCaretEventHub live. Luckily, all the callers to AccessibleCaretEventHub's public methods should add a ref to AccessibleCaretEventHub. (I'll double check this though.) All I need to do is to check shell->IsDestroying() after each FlushLayout() and return early, and all the other calls after that flush calls.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?from=PresShell%3A%3ADestroy%28%29#1049-1050

Comment 20

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

Comment on attachment 8718238 [details]
MozReview Request: Bug 1246918 - Fix carets missing after scrolling down in selection mode on Fennec. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34475/diff/2-3/

Comment 21

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

Attachment: MozReview Request: Bug 1246918 - Handle PresShell gone after FlushLayout(). r=roc

After calling FlushLayout(), PresShell::Destroy() might be called and we
should consider PreShell and other resources will be no longer valid.

Before this patch, AccessibleCaretManager and AccessibleCaret(s) are
deallocated in PresShell::Destroy(). However FlushLayout() are all
invoked in AccessibleCaretManager, we need to keep manager alive to
clean up after PresShell::Destroy().

This patch makes AccessibleCaretManager live after PresShell::Destroy(),
and use IsTerminated() to check whether PreShell is vaild after each
FlushLayout() calls.

Note that event though AccessibleCaretEventHub will be unref in
PresShell::Destroy(), all the callers to AccessibleCaretEventHub's
public methods already add a ref to AccessibleCaretEventHub. So we don't
need to worry about AccessibleCaretEventHub and AccessibleCaretManager
die immediately after PresShell::Destroy().

Review commit: https://reviewboard.mozilla.org/r/34957/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34957/

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8719392 [details]
MozReview Request: Bug 1246918 - Handle PresShell gone after FlushLayout(). r=roc

https://reviewboard.mozilla.org/r/34957/#review31525

Comment 23

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

Comment on attachment 8719392 [details]
MozReview Request: Bug 1246918 - Handle PresShell gone after FlushLayout(). r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34957/diff/1-2/

Comment 24

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #19)
> Mats, could you elaborate more on why nsAutoScriptBlocker could prevent
> PresShell::Destroy() being called?

A nsAutoScriptBlocker on the stack blocks "script runners" (see
nsContentUtils::AddScriptRunner) to the end of its scope.
When the top-most nsAutoScriptBlocker goes out of scope the queued
runners are executed.  As far as I know, a script runner is
the only thing that can lead to PresShell::Destroy() when you
flush it.  So adding a nsAutoScriptBlocker on the stack in
the (top-most) methods that can lead to a flush should be enough
to prevent your object from being deleted.

It only protects the shell that you flush though.  If your code
touches frames or sub-shells etc, then beware that those can be
destroyed merely by style changes (through the flush).

Comment 25

[Mats Palmgren (inactive)]

https://reviewboard.mozilla.org/r/34957/#review31573

::: layout/base/AccessibleCaretManager.cpp:204
(Diff revision 2)
> +  FlushLayout();
> +  if (IsTerminated()) {
> +    return;
> +  }
> +

I'm sorry, but I don't think this helps.  This code expands to:

FlushLayout();
if (!this->mPresShell)
  return;

The problem is that |this| might have been deleted when
we read its mPresShell member.

If you compare the old code, it doesn't have that problem:

nsCOMPtr<nsIPresShell> presShell = mPresShell;
FlushLayout();
if (presShell->IsDestroying())
  return;

because the strong ref on the stack keeps the shell alive
so that we can safely call IsDestroying() on it.

I don't see anything in this patch that keeps the AccessibleCaret*
objects alive that makes IsTerminated() safe to call.

Comment 26

[Mats Palmgren (inactive)]

Perhaps the most robust fix here is to add a nsAutoScriptBlocker in
all the On* methods in the AccessibleCaretEventHub::State sub-classes.
Are those the only entry points into the *Manager methods that flush?

Then you don't need to worry about |this| being deleted while you're
in one of these methods.  You only need to check that frames and such
are still alive after the flush but that's a simpler problem.

Comment 27

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

https://reviewboard.mozilla.org/r/34957/#review31573

> I'm sorry, but I don't think this helps.  This code expands to:
> 
> FlushLayout();
> if (!this->mPresShell)
>   return;
> 
> The problem is that |this| might have been deleted when
> we read its mPresShell member.
> 
> If you compare the old code, it doesn't have that problem:
> 
> nsCOMPtr<nsIPresShell> presShell = mPresShell;
> FlushLayout();
> if (presShell->IsDestroying())
>   return;
> 
> because the strong ref on the stack keeps the shell alive
> so that we can safely call IsDestroying() on it.
> 
> I don't see anything in this patch that keeps the AccessibleCaret*
> objects alive that makes IsTerminated() safe to call.

You're right, not everything is modified in this patch. I had explain some of the assumption in the description of this patch. Let me try to clarify.

The patch is tried to fix the exact issue you mentioned that |this| (AccessibleCaretManager) might have been deleted after PresShell::Destroy() is called.  Because upon PresShell::Destroy() is called, AccessblieCaretEventHub::Terminate() is called, and it will delete AccessibleCaretManager in [1].  Keep PresShell alive cannot gurantee that, so the the old code is still dangerous.

Instead, this patch makes AccessblieCaretEventHub::Terminate() calls AccessibleCaretManager::Terminate() which sets mPresShell to nullptr. This has two effects. 1) It made the life cycle of AccessibleCaretEventHub and AccessibleCaretManager the same. 2) We can safely call AccessibleCaretManager::IsTerminated() to know whether PresShell::Destroy() had been called without holding a reference to mPresShell to use its IsDestroying().

The remaining question is who keep AccessibleCaretEventHub alive after calling PresShell::Destroy() since Destroy() will unref it. Luckily all the entry points [2] to AccessibleCaretEventHub do add ref to it. Those On\* methods in the AccessibleCaretEventHub::State sub-classes are not the actual entry points which might flush layout. There's the complete list of the top-level public entry points and where the ref is added.

1) AccessibleCaretEventHub::HandleEvent() https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/layout/base/nsPresShell.cpp#6888-6889,6891
2) AccessibleCaretEventHub::NotifyBlur() https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/dom/base/nsFocusManager.cpp#1679,1681
3) AccessibleCaretEventHub::Reflow() and AccessibleCaretEventHub::ReflowInterruptible() https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/docshell/base/nsDocShell.cpp#2354,2358,2360
4) AccessibleCaretEventHub::AsyncPanZoomStarted() and AccessibleCaretEventHub::AsyncPanZoomStopped() https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/docshell/base/nsDocShell.cpp#3051,3053,3075,3077
5) AccessibleCaretEventHub::ScrollPositionChanged() https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/docshell/base/nsDocShell.cpp#3099,3101

With all of these, I think both AccessibleCaretEventHub and AccessibleCaretManager will be alive until the above public methods returned even if some flush layout calls down to the stack which might call PresShell::Destroy() or might make PresShell deleted.

[1] https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/layout/base/AccessibleCaretEventHub.cpp#442,462

Comment 28

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

Mats, does comment #27 make some sense to you? :)

Comment 29

[Mats Palmgren (inactive)]

Ah, I wasn't aware we were already holding a strong ref on the AccessibleCaretEventHub
on the stack at all entry points.  Yeah, this should work now that you also keep the Manager
alive and use Terminate/IsTerminated instead.  Thanks for sorting this out!

I wonder if it would be worth adding some kind of (DEBUG-only) mechanism to detect any
violations of this precondition, by asserting in the Manager code that the Hub's mRefCnt > 1
(the shell's reference plus at least one more).  Perhaps that's overkill though.

Comment 30

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

Good point. It should be worth adding debug-only mechanism for Hub's mRefCnt > 1 because this is the essential precondition that makes all this work. Since Manager cannot direct access Hub unless it get Hub's reference from PresShell, maybe adding assert in all the entry points in Hub should be as good. Let's land this and do it in a follow up bug later.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3e37b63def
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e71da98b14

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc3e37b63def
https://hg.mozilla.org/mozilla-central/rev/10e71da98b14

Comment 33

[Carsten Book [:Tomcat]]

sorry had to back this out, seems this caused some frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=21822300&repo=mozilla-inbound 

could you take a look, thanks !

Comment 34

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/7ced13d644be
https://hg.mozilla.org/mozilla-central/rev/709f559b5406

Comment 35

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

By looking at treeherder result of the backout in comment #34, the c1 failure on Android still happens. Since my patch does not touch media code, is it possible the crash is related to other changes? Is it OK to reland my patches?

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=709f559b5406

Comment 36

[Carsten Book [:Tomcat]]

yeah feel free to reland

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f02725af4ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d19f450495

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9f02725af4ad
https://hg.mozilla.org/mozilla-central/rev/10d19f450495