Closed Bug 1300858 Opened 3 years ago Closed 3 years ago

Attempted reentry into chrome via Accessible::TakeFocus

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

TL;DR: Since Windows a11y+e10s may be triggered while waiting on a sync message, we can get into a situation where TakeFocus() tries to send while the message channel is still waiting on a sync reply. Can we dispatch xul!nsFocusManager::SetFocus from an event posted to the main thread?

See this call stack:

xul!mozilla::ipc::MessageChannel::Send+0x378
xul!mozilla::dom::PContentChild::SendClipboardHasType+0x139
xul!nsClipboardProxy::HasDataMatchingFlavors+0x80
xul!mozilla::TextEditor::CanPaste+0x134
xul!mozilla::PasteCommand::IsCommandEnabled+0x13a
xul!nsControllerCommandTable::IsCommandEnabled+0xcd
xul!nsBaseCommandController::IsCommandEnabled+0x100
xul!nsWindowRoot::GetEnabledDisabledCommandsForControllers+0x162
xul!nsWindowRoot::GetEnabledDisabledCommands+0x6d
xul!ChildCommandDispatcher::Run+0x8f
xul!nsContentUtils::AddScriptRunner+0x87
xul!nsContentUtils::AddScriptRunner+0x1f
xul!nsGlobalWindow::UpdateCommands+0x99
xul!nsFocusManager::Focus+0x9cc
xul!nsFocusManager::SetFocusInner+0xba0
xul!nsFocusManager::SetFocus+0xb6
xul!mozilla::a11y::Accessible::TakeFocus+0x10f
xul!mozilla::a11y::AccessibleWrap::accSelect+0xcd
ole32!CallFrame::Invoke+0x63
xul!`anonymous namespace'::HandoffRunnable::Run+0x1e
xul!`anonymous namespace'::SyncRunnable::Run+0x20
xul!mozilla::mscom::MainThreadInvoker::MainThreadAPC+0x93
ntdll!RtlDispatchAPC+0x6a195
ntdll!KiUserApcDispatcher+0x2d
USER32!MsgWaitForMultipleObjectsEx+0x17b
combase!CCliModalLoop::BlockFn+0x11c
combase!ClassicSTAThreadWaitForHandles+0xb4
combase!CoWaitForMultipleHandles+0x8e
xul!mozilla::ipc::MessageChannel::WaitForSyncNotifyWithA11yReentry+0x107
xul!mozilla::ipc::MessageChannel::WaitForSyncNotify+0xed
xul!mozilla::ipc::MessageChannel::Send+0x909
xul!mozilla::net::PCookieServiceChild::SendGetCookieString+0x173
xul!mozilla::net::CookieServiceChild::GetCookieStringInternal+0x19f
xul!mozilla::net::CookieServiceChild::GetCookieString+0x1c
xul!nsHTMLDocument::GetCookie+0x198
xul!mozilla::dom::HTMLDocumentBinding::get_cookie+0x49
xul!mozilla::dom::GenericBindingGetter+0x1b0
See the bug description.
Flags: needinfo?(tbsaunde+mozbugs)
Yeah, I think it would be fine to dispatch TakeFocus() from an event posted to the main thread.
Flags: needinfo?(tbsaunde+mozbugs)
Attached patch Patch (obsolete) — Splinter Review
This patch only dispatches to the main thread on Windows e10s content processes. In other situations it maintains the current behaviour.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8789012 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8789012 [details] [diff] [review]
Patch

>+    if (XRE_IsContentProcess()) {
>+      // In this case it may be possible for TakeFocus() to be invoked while
>+      // the IPC MessageChannel is waiting on a sync reply. We cannot dispatch
>+      // additional IPC while that is happening, so we dispatch the SetFocus
>+      // from the main thread to guarantee that we are outside any IPC.
>+      nsCOMPtr<nsIRunnable> runnable =
>+        mozilla::NewRunnableMethod<nsCOMPtr<nsIDOMElement>, uint32_t>(fm,
>+          &nsIFocusManager::SetFocus, element, 0U);
>+      NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL);

you also need to worry about DocAccessible::TakeFocus() its probably best to dispatch all of the call to TakeFocus() to a runnable.
Attachment #8789012 - Flags: review?(tbsaunde+mozbugs)
Attached patch Patch (r2)Splinter Review
Attachment #8789012 - Attachment is obsolete: true
Attachment #8789034 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8789034 [details] [diff] [review]
Patch (r2)

Review of attachment 8789034 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +836,5 @@
>    if (xpAccessible->IsDefunct())
>      return CO_E_OBJNOTCONNECTED;
>  
>    if (flagsSelect & SELFLAG_TAKEFOCUS) {
> +    if (XRE_IsContentProcess()) {

it probably makes sense to run it always through runnable not depending on a process, for consistence

@@ +851,5 @@
>      return S_OK;
>    }
>  
>    if (flagsSelect & SELFLAG_TAKESELECTION) {
>      xpAccessible->TakeSelection();

selection changes might be another issue that should be resolved same way
(In reply to alexander :surkov from comment #6)
> Comment on attachment 8789034 [details] [diff] [review]
> Patch (r2)
> 
> Review of attachment 8789034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ +836,5 @@
> >    if (xpAccessible->IsDefunct())
> >      return CO_E_OBJNOTCONNECTED;
> >  
> >    if (flagsSelect & SELFLAG_TAKEFOCUS) {
> > +    if (XRE_IsContentProcess()) {
> 
> it probably makes sense to run it always through runnable not depending on a
> process, for consistence

maybe, but I'm not sure I care either way.

> @@ +851,5 @@
> >      return S_OK;
> >    }
> >  
> >    if (flagsSelect & SELFLAG_TAKESELECTION) {
> >      xpAccessible->TakeSelection();
> 
> selection changes might be another issue that should be resolved same way

Well, eventually you will probably need to look at all the things on PDocAccessible that are async, but this one is fine for now.
Attachment #8789034 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> > >    if (flagsSelect & SELFLAG_TAKESELECTION) {
> > >      xpAccessible->TakeSelection();
> > 
> > selection changes might be another issue that should be resolved same way
> 
> Well, eventually you will probably need to look at all the things on
> PDocAccessible that are async, but this one is fine for now.

good, it'd be nice though to keep things consistent, so that both TakeFocus()/TakeSelection() are handled similar way, since they both end up with triggering same code.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1716e1d3741cf09ac579194ddf4b5125df0ca9
Bug 1300858: Make AccessibleWrap::accSelect dispatch TakeFocus() call to main thread when running in Windows e10s content process; r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/ab1716e1d374
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.