1338755 - 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:

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Treeherder Bug Filer]

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()".

Comment 1

[Trevor Saunders (:tbsaunde)]

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.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: split ParentCOMProxy message up

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.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: split parts of SetCOMProxy() out to MybeInitWindowEmulation()

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: split up SetCOMProxy()

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: make RemoteChildDoc() return a DocAccessibleParent*

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: send a tabs parent proxy when it may have changed

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

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)

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Trevor, see comment 7

Comment 9

[Trevor Saunders (:tbsaunde)]

(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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d7ba5c9105
split ParentCOMProxy message up r=yzen

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: split parts of SetCOMProxy() out to MaybeInitWindowEmulation()

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: split up SetCOMProxy()

Comment 13

[Intermittent Failures Robot]

24 failures in 151 pushes (0.159 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* autoland: 11
* mozilla-inbound: 10
* mozilla-central: 2
* graphics: 1

Platform breakdown:
* windows8-64: 20
* windows7-32-vm: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1338755&startday=2017-02-16&endday=2017-02-16&tree=all

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

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 }

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8838205 [details] [diff] [review]
split up SetCOMProxy()

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

looks good

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05d7ba5c9105

Comment 17

[Trevor Saunders (:tbsaunde)]

(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()

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/162b85cf472d
https://hg.mozilla.org/mozilla-central/rev/874b53206690
https://hg.mozilla.org/mozilla-central/rev/7fcdfa7a42b7
https://hg.mozilla.org/mozilla-central/rev/e66b5929cc7d

Comment 20

[Intermittent Failures Robot]

24 failures in 180 pushes (0.133 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* autoland: 11
* mozilla-inbound: 10
* mozilla-central: 2
* graphics: 1

Platform breakdown:
* windows7-32-vm: 15
* windows8-64: 9

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1338755&startday=2017-02-17&endday=2017-02-17&tree=all

Comment 21

[Intermittent Failures Robot]

74 failures in 833 pushes (0.089 failures/push) were associated with this bug in the last 7 days. 

This is the #28 most frequent failure this week. 

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:
* mozilla-inbound: 34
* autoland: 29
* mozilla-central: 7
* try: 2
* graphics: 2

Platform breakdown:
* windows8-64: 43
* windows7-32-vm: 31

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1338755&startday=2017-02-13&endday=2017-02-19&tree=all