Closed Bug 1338755 Opened 7 years ago Closed 7 years ago

Intermittent PROCESS-CRASH | Main app process exited normally | application crashed [@ mozilla::a11y::DocAccessibleChild::RecvParentCOMProxy(mozilla::mscom::COMPtrHolder<IAccessible,&_GUID const IID_IAccessible> const &,unsigned __int64 const &,mozilla:

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: tbsaunde)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed])

Attachments

(5 files, 2 obsolete files)

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=76521962&repo=mozilla-inbound

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win64-debug/1486769701/mozilla-inbound_win8_64-debug_test-mochitest-e10s-browser-chrome-3-bm109-tests1-windows-build136.txt.gz

Had you left me any summary room with a decent-sized crashing frame, I would have mentioned "after Assertion failure: !mParentProxy && !aParentCOMProxy.IsNull()".
so this is the same basic issue as bug 1337983 accept what happens when we remove that assert.  So it should get fixed soon as part of dealing with the  real issue there.
Summary: Intermittent Main app process exited normally | application crashed [@ mozilla::a11y::DocAccessibleChild::RecvParentCOMProxy(mozilla::mscom::COMPtrHolder<IAccessible,&_GUID const IID_IAccessible> const &,unsigned __int64 const &,mozilla::mscom::COMPtrHo → Intermittent PROCESS-CRASH | Main app process exited normally | application crashed [@ mozilla::a11y::DocAccessibleChild::RecvParentCOMProxy(mozilla::mscom::COMPtrHolder<IAccessible,&_GUID const IID_IAccessible> const &,unsigned __int64 const &,mozilla:
sending the emulated window information is basically unrelated to sending the
parent COM proxy to the child process, and in the future it will be useful to
send these at different times.
Attachment #8837779 - Flags: review?(yzenevich)
Attached patch split up SetCOMProxy() (obsolete) — Splinter Review
Attachment #8837781 - Flags: review?(yzenevich)
Attachment #8837779 - Flags: review?(yzenevich) → review+
Comment on attachment 8837780 [details] [diff] [review]
split parts of SetCOMProxy() out to MybeInitWindowEmulation()

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

::: dom/ipc/TabParent.cpp
@@ +961,5 @@
>  
>      doc->SetTopLevel();
>      a11y::DocManager::RemoteDocAdded(doc);
>  #ifdef XP_WIN
> +    doc->MaybeInitWindowEmulation();

As far as I remember, setting emulation should happen after the COM proxy is set. The reason is that we sometimes calculate bounds for the proxy (https://dxr.mozilla.org/mozilla-central/source/accessible/ipc/DocAccessibleParent.cpp#556) and for that we need the COM proxy set (https://dxr.mozilla.org/mozilla-central/source/accessible/ipc/win/ProxyAccessible.cpp#210)
Trevor, see comment 7
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Yura Zenevich [:yzen] from comment #7)
> Comment on attachment 8837780 [details] [diff] [review]
> split parts of SetCOMProxy() out to MybeInitWindowEmulation()
> 
> Review of attachment 8837780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ +961,5 @@
> >  
> >      doc->SetTopLevel();
> >      a11y::DocManager::RemoteDocAdded(doc);
> >  #ifdef XP_WIN
> > +    doc->MaybeInitWindowEmulation();
> 
> As far as I remember, setting emulation should happen after the COM proxy is
> set. The reason is that we sometimes calculate bounds for the proxy
> (https://dxr.mozilla.org/mozilla-central/source/accessible/ipc/
> DocAccessibleParent.cpp#556) and for that we need the COM proxy set
> (https://dxr.mozilla.org/mozilla-central/source/accessible/ipc/win/
> ProxyAccessible.cpp#210)

erg yeah, that's my bad.  Shouldn't be hard to fix but I need to go rearrange this series of patches.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8837780 - Attachment is obsolete: true
Attachment #8837780 - Flags: review?(yzenevich)
Attachment #8838205 - Flags: review?(yzenevich)
Attachment #8837781 - Attachment is obsolete: true
Attachment #8837781 - Flags: review?(yzenevich)
Comment on attachment 8838200 [details] [diff] [review]
split parts of SetCOMProxy() out to MaybeInitWindowEmulation()

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

looks good, 1 nit and 1 question

::: accessible/ipc/DocAccessibleParent.cpp
@@ +559,5 @@
> +    nsIntRect rootRect = rootDocument->Bounds();
> +    rect.x = rootRect.x - rect.x;
> +    rect.y -= rootRect.y;
> +
> +    auto tab = static_cast<dom::TabParent*>(Manager());

would we need to bail if tab is destroyed?

@@ +579,5 @@
> +                                               IID_IAccessible,
> +                                               (void**)&rawHWNDAcc))) {
> +      hWndAccHolder.Set(IAccessibleHolder::COMPtrType(rawHWNDAcc));
> +    }
> +    }

nit: extra white space before }
Attachment #8838200 - Flags: review?(yzenevich) → review+
Comment on attachment 8838205 [details] [diff] [review]
split up SetCOMProxy()

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

looks good
Attachment #8838205 - Flags: review?(yzenevich) → review+
Attachment #8837782 - Flags: review?(yzenevich) → review+
Attachment #8837783 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/05d7ba5c9105
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Yura Zenevich [:yzen] from comment #14)
> Comment on attachment 8838200 [details] [diff] [review]
> split parts of SetCOMProxy() out to MaybeInitWindowEmulation()
> 
> Review of attachment 8838200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good, 1 nit and 1 question
> 

> @@ +559,5 @@
> > +    nsIntRect rootRect = rootDocument->Bounds();
> > +    rect.x = rootRect.x - rect.x;
> > +    rect.y -= rootRect.y;
> > +
> > +    auto tab = static_cast<dom::TabParent*>(Manager());
> 
> would we need to bail if tab is destroyed?

no, the code that does that is non redundant see what happens in RecvPDocAccessibleConstructor()
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/162b85cf472d
split parts of SetCOMProxy() out to MaybeInitWindowEmulation() r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/874b53206690
split up SetCOMProxy() r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcdfa7a42b7
make RemoteChildDoc() return a DocAccessibleParent* r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66b5929cc7d
send a tab's parent proxy when it may have changed r=yzen
Assignee: nobody → tbsaunde+mozbugs
Whiteboard: [stockwell fixed]
You need to log in before you can comment on or make changes to this bug.