Reset accessibility if it gets out of sync.

Reset accessibility if it gets out of sync.

Previously if the browser got an accessibility IPC it was unable to
interpret, it killed the renderer. This change makes it reset the
accessibility state instead so we're tolerant of corner cases that
are very difficult to completely eliminate in practice.

The accessibility reset is designed to be safe, it throws away all
accessibility state on the browser side and waits for an IPC from
the renderer acknowledging the renderer reset as well.

BUG=372478
Committed: https://crrev.com/0c5e8d16661df7e08c708997f24c0a3070e58080
Cr-Commit-Position: refs/heads/master@{#298297}

Committed: https://crrev.com/59ed1bba589383443823e35802775bee75c2ee9a
Cr-Commit-Position: refs/heads/master@{#301106}

[dmazzoni, 2014-10-02 05:14:27]

dmazzoni@chromium.org changed reviewers:
+ nasko@chromium.org, tsepez@chromium.org

[dmazzoni, 2014-10-02 05:19:11]

tsepez: this is based on our previous conversation about how to deal with these
cases that crash the renderer with no useful info.

nasko: would you please review the rest, since you're familiar with RFHI and how
it interacts with accessibility?

Thanks!

[Mike West, 2014-10-02 06:27:47]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-02 06:27:48]

Looks pretty reasonable, two drive-by comments below.

https://codereview.chromium.org/625443002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1053: LOG(ERROR) << " ***
Browser skipping *** ";
Perhaps you could consider crashing the renderer if you get a reset message
while you're not waiting for a reset. That sounds like a good way for the
browser and renderer to get out of sync.

Also a nit: The log message should probably contain detail about what the
browser is skipping.

https://codereview.chromium.org/625443002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1406: return NULL;
Nit: nullptr?

[dmazzoni, 2014-10-02 06:40:18]

Thanks!

https://codereview.chromium.org/625443002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1053: LOG(ERROR) << " ***
Browser skipping *** ";
On 2014/10/02 06:27:48, Mike West wrote:
> Perhaps you could consider crashing the renderer if you get a reset message
> while you're not waiting for a reset. That sounds like a good way for the
> browser and renderer to get out of sync.

Well, part of the point of this change is to avoid crashing the renderer,
because when we do it's almost impossible to decipher the crash logs. I'm hoping
that just ignoring those IPCs is pretty safe, that prevents them from being used
as an attack vector.

> Also a nit: The log message should probably contain detail about what the
> browser is skipping.

Oops, I didn't mean to leave the log in at all.

[nasko, 2014-10-02 16:38:48]

A few comments.

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:77:
AccessibilityNotificationWaiter waiter2(
Since you don't preserve any state in each of those wait blocks, why not enclose
them in local scope (between {}) that way you don't need to have waiterX as you
have more of these blocks. It also makes it easier to read/follow, since it is
clear that this is a unit of code.

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:87: //
Ensure that we didn't kill the renderer; we can still send it messages.
Do you plan to test that the process kill functionality works?

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:468:
accessibility_reset_count_++;
This is never reset to 0, even when the renderer is killed. The user can reload
a tab with killed renderer, but RFH will not be recreated for the top-level
frame, so this will just keep on killing the renderer.

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:472: Send(new
AccessibilityMsg_Reset(routing_id_));
Do you want to UMA stat how often this happens? According to the comments, it
should be very rare, so some concrete confirmation through data might be useful.

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:489: bool
waiting_on_accessibility_reset_;
It will be nice to think about whether all accessibility code can be moved to a
separate object that RFH owns. One of my worries is RFH becoming a monster of an
object in the long run. Nothing to do for this CL, but some thinking to do as we
complete the out-of-process iframes transition for accessibility.

[Tom Sepez, 2014-10-02 16:50:50]

https://codereview.chromium.org/625443002/diff/60001/content/common/accessibi...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/625443002/diff/60001/content/common/accessibi...
content/common/accessibility_messages.h:157: bool /* is_reset */)
Are you sure you want a bool instead of a reset sequence number?  I could
imagine two resets overlapping somehow -- in other words, you've still got
implied shared state between the renderer and browser that you could make
explicit.

[dmazzoni, 2014-10-02 21:51:13]

Ready for another look!

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:77:
AccessibilityNotificationWaiter waiter2(
On 2014/10/02 16:38:47, nasko wrote:
> Since you don't preserve any state in each of those wait blocks, why not
enclose
> them in local scope (between {}) that way you don't need to have waiterX as
you
> have more of these blocks. It also makes it easier to read/follow, since it is
> clear that this is a unit of code.

Good idea, done.

https://codereview.chromium.org/625443002/diff/60001/content/browser/accessib...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:87: //
Ensure that we didn't kill the renderer; we can still send it messages.
On 2014/10/02 16:38:48, nasko wrote:
> Do you plan to test that the process kill functionality works?

Sure, makes sense. I added a new test.

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:468:
accessibility_reset_count_++;
On 2014/10/02 16:38:48, nasko wrote:
> This is never reset to 0, even when the renderer is killed. The user can
reload
> a tab with killed renderer, but RFH will not be recreated for the top-level
> frame, so this will just keep on killing the renderer.

Good point. I reset it in OnDidStartProvisionalLoadForFrame, in manual testing
that seemed to work if I reload a page but not otherwise. Does that look good or
can you think of a better place?

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:472: Send(new
AccessibilityMsg_Reset(routing_id_));
On 2014/10/02 16:38:48, nasko wrote:
> Do you want to UMA stat how often this happens? According to the comments, it
> should be very rare, so some concrete confirmation through data might be
useful.

Great idea. I added three counts that should be enough to give us a good sense
of how often this is happening.

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:489: bool
waiting_on_accessibility_reset_;
On 2014/10/02 16:38:48, nasko wrote:
> It will be nice to think about whether all accessibility code can be moved to
a
> separate object that RFH owns. One of my worries is RFH becoming a monster of
an
> object in the long run. Nothing to do for this CL, but some thinking to do as
we
> complete the out-of-process iframes transition for accessibility.

Sure, I've been thinking about this too. If you think it'd be okay for it to be
a sibling class (i.e. dependencies can go both directions) then it'd be super
easy.

https://codereview.chromium.org/625443002/diff/60001/content/common/accessibi...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/625443002/diff/60001/content/common/accessibi...
content/common/accessibility_messages.h:157: bool /* is_reset */)
On 2014/10/02 16:50:49, Tom Sepez wrote:
> Are you sure you want a bool instead of a reset sequence number?  I could
> imagine two resets overlapping somehow -- in other words, you've still got
> implied shared state between the renderer and browser that you could make
> explicit.

I was debating this. I went ahead and gave it a random reset token rather than a
sequence that could easily be guessed, just to be safe.

[Tom Sepez, 2014-10-02 21:56:49]

https://codereview.chromium.org/625443002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:494:
accessibility_reset_token_ = base::RandInt(1, INT_MAX);
This is probably overkill, and there is always the possibility of a collision
now that you've got random numbers.  I'm not worried about the renderer guessing
this ...

[dmazzoni, 2014-10-02 22:01:24]

https://codereview.chromium.org/625443002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:494:
accessibility_reset_token_ = base::RandInt(1, INT_MAX);
On 2014/10/02 21:56:49, Tom Sepez wrote:
> This is probably overkill, and there is always the possibility of a collision
> now that you've got random numbers.  I'm not worried about the renderer
guessing
> this ... 

No problem, replaced with sequential.

[Tom Sepez, 2014-10-02 22:03:30]

lgtm

[nasko, 2014-10-03 15:58:41]

Mostly nits.

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:160: for
(int iteration = 0; iteration < 5; iteration++) {
Why not define the constant in a header and use it here? This seems error prone
if we want to adjust the value at a later point in time.

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:180:
ASSERT_FALSE(ExecuteScript(
Is this extra step needed? There is
RenderProcessHostObserver::RenderProcessExited event that can be monitored.

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:664: void
RenderFrameHostImpl::OnDidStartProvisionalLoadForFrame(
I'd move this to OnDidCommitProvisionalLoad, as we can start a load but not
commit it - fail or download, at which point we stay at the current document.

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:570: // we're waiting for an
IPC containing this reset token (randomly chosen)
nit: with the latest CL it is a sequential one, not randomly chosen, right?

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_complete.cc:233:
reset_token_ = 0;
nit: I'd move this assignment in the OnReset, so when one reads the method it is
clear that reset_token_ is non-zero only for the duration of that method.

[dmazzoni, 2014-10-03 19:08:12]

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:160: for
(int iteration = 0; iteration < 5; iteration++) {
On 2014/10/03 15:58:41, nasko wrote:
> Why not define the constant in a header and use it here? This seems error
prone
> if we want to adjust the value at a later point in time.

Done.

https://codereview.chromium.org/625443002/diff/120001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:180:
ASSERT_FALSE(ExecuteScript(
On 2014/10/03 15:58:41, nasko wrote:
> Is this extra step needed? There is
> RenderProcessHostObserver::RenderProcessExited event that can be monitored.

Done.

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:664: void
RenderFrameHostImpl::OnDidStartProvisionalLoadForFrame(
On 2014/10/03 15:58:41, nasko wrote:
> I'd move this to OnDidCommitProvisionalLoad, as we can start a load but not
> commit it - fail or download, at which point we stay at the current document.

Makes sense. It looks like most of OnDidCommitProvisionalLoad is error checking
so I put it at the bottom. Please double-check that makes sense to you.

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:570: // we're waiting for an
IPC containing this reset token (randomly chosen)
On 2014/10/03 15:58:41, nasko wrote:
> nit: with the latest CL it is a sequential one, not randomly chosen, right?

Done.

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_complete.cc:233:
reset_token_ = 0;
On 2014/10/03 15:58:41, nasko wrote:
> nit: I'd move this assignment in the OnReset, so when one reads the method it
is
> clear that reset_token_ is non-zero only for the duration of that method.

Unfortunately that won't work - HandleAXEvent doesn't run synchronously, it
posts a task.

[nasko, 2014-10-06 16:39:03]

LGTM with some nits.

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/625443002/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_complete.cc:233:
reset_token_ = 0;
On 2014/10/03 19:08:12, dmazzoni wrote:
> On 2014/10/03 15:58:41, nasko wrote:
> > nit: I'd move this assignment in the OnReset, so when one reads the method
it
> is
> > clear that reset_token_ is non-zero only for the duration of that method.
> 
> Unfortunately that won't work - HandleAXEvent doesn't run synchronously, it
> posts a task.

Acknowledged.

https://codereview.chromium.org/625443002/diff/160001/content/browser/accessi...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:74:
ASSERT_TRUE(frame->GetOrCreateBrowserAccessibilityManager() != NULL);
nit: It will be nice to start using nullptr instead of NULL now that we have
C++11 support.

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:760:
accessibility_reset_count_ = 0;
It will be better to move this line above the DidNavigate call, which updates
bunch of state on RFH/RFHM.

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:368: static const int
kMaxAccessibilityResets = 5;
Please move this up next to the enum at the beginning of the class.

[dmazzoni, 2014-10-06 16:44:02]

https://codereview.chromium.org/625443002/diff/160001/content/browser/accessi...
File content/browser/accessibility/accessibility_ipc_error_browsertest.cc
(right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/accessi...
content/browser/accessibility/accessibility_ipc_error_browsertest.cc:74:
ASSERT_TRUE(frame->GetOrCreateBrowserAccessibilityManager() != NULL);
On 2014/10/06 16:39:02, nasko wrote:
> nit: It will be nice to start using nullptr instead of NULL now that we have
> C++11 support.

Changed throughout.

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:760:
accessibility_reset_count_ = 0;
On 2014/10/06 16:39:03, nasko wrote:
> It will be better to move this line above the DidNavigate call, which updates
> bunch of state on RFH/RFHM.

Done.

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/625443002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:368: static const int
kMaxAccessibilityResets = 5;
On 2014/10/06 16:39:03, nasko wrote:
> Please move this up next to the enum at the beginning of the class.

Done.

[dmazzoni, 2014-10-06 16:44:43]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-06 16:45:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625443002/180001

[commit-bot: I haz the power, 2014-10-06 19:57:20]

Committed patchset #10 (id:180001) as b9477777b4d0ba5574b8f5e36fa4557f7fa4c9f9

[commit-bot: I haz the power, 2014-10-06 19:58:23]

Patchset 10 (id:??) landed as
https://crrev.com/0c5e8d16661df7e08c708997f24c0a3070e58080
Cr-Commit-Position: refs/heads/master@{#298297}

[jackhou1, 2014-10-07 00:23:48]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/629413002/ by jackhou@chromium.org.

The reason for reverting is: Suspecting this for failing test:

https://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28db....

[dmazzoni, 2014-10-23 21:56:23]

Oops, forgot this got reverted. Reopening to fix test failure and re-land.

[Mike West, 2014-10-24 09:31:23]

On 2014/10/23 21:56:23, dmazzoni wrote:
> Oops, forgot this got reverted. Reopening to fix test failure and re-land.

I think we usualy just open a new issue. One commit, one review is a good rule
of thumb.

[dmazzoni, 2014-10-24 15:00:09]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-24 15:01:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625443002/220001

[commit-bot: I haz the power, 2014-10-24 15:47:34]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-10-24 15:48:47]

Patchset 12 (id:??) landed as
https://crrev.com/59ed1bba589383443823e35802775bee75c2ee9a
Cr-Commit-Position: refs/heads/master@{#301106}

[dmazzoni, 2014-10-24 16:02:51]

On Fri, Oct 24, 2014 at 2:31 AM, <mkwst@chromium.org> wrote:

> On 2014/10/23 21:56:23, dmazzoni wrote:
>
>> Oops, forgot this got reverted. Reopening to fix test failure and re-land.
>>
>
> I think we usualy just open a new issue. One commit, one review is a good
> rule
> of thumb.
>

There was a big debate on this on chromium-dev - I thought there was no
consensus reached, some people like the same issue, others like a new one?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.