1489317 - Crash in OXIDEntry::StartServer

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-829d8200-3d63-4bb7-94c0-586920180906.
=============================================================

Top 10 frames of crashing thread:

0 ole32.dll OXIDEntry::StartServer 
1 ole32.dll CComApartment::StartServer 
2 ole32.dll AppInvoke 
3 ole32.dll CDllHost::GetApartmentToken 
4 ole32.dll CClassCache::GetActivatorFromDllHost 
5 ole32.dll CProcessActivator::GetApartmentActivator 
6 ole32.dll CProcessActivator::CCICallback 
7 ole32.dll CProcessActivator::AttemptActivation 
8 ole32.dll CProcessActivator::ActivateByContext 
9 ole32.dll CProcessActivator::CreateInstance 

=============================================================

this crash signature is becoming somewhat more common during the 63 cycle on windows 7. the first nightly exhibiting this was 63.0a1 build 20180719220047.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

This looks like fallout from bug 1425144.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

I'll take this, as it actually requires some mscom magic.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1489317: Part 1 - Add forward declaration for mscom::AgileReference to RefPtr; r=froydnj!

I'd like to add a constructor and operator= to RefPtr for mscom::AgileReference.
This patch is simply the forward declarations to allow for that.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1489317: Part 2 - Improvements to mscom::AgileReference; r=froydnj!

This patch adds the definitions of the RefPtr constructor and operator=.
It also refactors some stuff in AgileReference to make these objects easier
to use. Since it's just a bunch of C++ goop, I figured that you'd be fine to
review this. Let me know if you want to add a reviewer who is more familiar
with the COM nuances.

Depends on D5317

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell!

Depends on D5318

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1489317: Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell!

Depends on D5321

Comment 7

[Molly Howell (she/her) (no longer active)]

Comment on attachment 9007365 [details]
Bug 1489317: Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell!

Matt Howell [:mhowell] has approved the revision.

Comment 8

[Molly Howell (she/her) (no longer active)]

Comment on attachment 9007363 [details]
Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell!

Matt Howell [:mhowell] has approved the revision.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 9007359 [details]
Bug 1489317: Part 1 - Add forward declaration for mscom::AgileReference to RefPtr; r=froydnj!

Nathan Froyd [:froydnj] has approved the revision.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9007360 [details]
Bug 1489317: Part 2 - Improvements to mscom::AgileReference; r=froydnj!

Nathan Froyd [:froydnj] has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f8554d82b32
Part 1 - Add forward delcaration for mscom::AgileReference to RefPtr; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e0535e0450c8
Part 2 - Improvements to mscom::AgileReference; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9d444f92b939
Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell
https://hg.mozilla.org/integration/autoland/rev/8cf95604ce94
Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

If this shows up in perf profiles again, then we'll have to be even more aggressive and only create/interact with ICustomDestinationList objects off main thread, in a multi-threaded apartment. I checked the registry configuration for this interface's Proxy/Stub class, and its threading model is classified as "Both."

We'll hold off on that for now, unless we see more perf issues crop up.

Comment 13

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1489317) for causing Windows test failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8cf95604ce946196367410a3987377ec32b492c6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

https://hg.mozilla.org/integration/autoland/rev/b9c9088a486865ec1a5275a5a9bd960cfd7e36a1

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2787fb454f40
Part 1 - Add forward delcaration for mscom::AgileReference to RefPtr; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/62b404e59c32
Part 2 - Improvements to mscom::AgileReference; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/b340f5acb7b5
Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell
https://hg.mozilla.org/integration/autoland/rev/07b44bee061c
Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell

Comment 15

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1489317) for windows test failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=07b44bee061cf080e70e6872f5f15e22e23819e0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&filter-searchStr=windows

backout: https://hg.mozilla.org/integration/autoland/rev/91136930be6477b2c2495e2045874c437a9a94bb

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fddf77dc2f9d
Part 1 - Add forward delcaration for mscom::AgileReference to RefPtr; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d5d432fd2433
Part 2 - Improvements to mscom::AgileReference; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7528056ab631
Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell
https://hg.mozilla.org/integration/autoland/rev/e35a9b02675b
Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Ugh. Okay, so there are problems because of that recursive mutex.

On the lazy idle thread, we make a call to the proxied interface. That call now needs the main thread to pump messages.

Meanwhile, the main thread is trying to call a different method on the jump list builder, and is blocked on the recursive mutex.

We either need to replace the Gecko recursive mutex such that we can call CoWaitForMultipleHandles, or we need to do the full refactoring of this code to use the multithreaded apartment.

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/151b11e40af6
Backed out 4 changesets for causing multiple windows tests timeouts. CLOSED TREE

Comment 19

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=e35a9b02675b53de79540d8623a80407214885d2&tochange=151b11e40af6f9ce57519999f228887fa243a292&selectedJob=198573006

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=198573006&repo=autoland&lineNumber=5976
https://treeherder.mozilla.org/logviewer.html#?job_id=198573007&repo=autoland&lineNumber=26089
https://treeherder.mozilla.org/logviewer.html#?job_id=198572609&repo=autoland&lineNumber=40964
https://treeherder.mozilla.org/logviewer.html#?job_id=198574580&repo=autoland&lineNumber=7118

Backout link: https://hg.mozilla.org/integration/autoland/rev/151b11e40af6f9ce57519999f228887fa243a292

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #17)
> We either need to replace the Gecko recursive mutex such that we can call
> CoWaitForMultipleHandles, or we need to do the full refactoring of this code
> to use the multithreaded apartment.

There's a third option. We create the COM object in the MTA.

That probably requires the least amount of code churn, provided that it works as expected...

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/552e33e562c3
Part 1 - Add forward declaration for mscom::AgileReference to RefPtr; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9c47b5b35399
Part 2 - Improvements to mscom::AgileReference; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7b7af15e22ed
Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell
https://hg.mozilla.org/integration/autoland/rev/e03c9835c643
Part 4 - Add asssertion that COM is initialized on the thread that is resolving an agile reference; r=mhowell

Comment 23

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/552e33e562c3
https://hg.mozilla.org/mozilla-central/rev/9c47b5b35399
https://hg.mozilla.org/mozilla-central/rev/7b7af15e22ed
https://hg.mozilla.org/mozilla-central/rev/e03c9835c643

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Aaron, could your request an uplift to beta when you get a chance? We could uplift it after checking that the crashes are fixed on nightly.Thanks

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9007363 [details]
Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell!

Approval Request Comment
[Feature/Bug causing the regression]: bug 1425144
[User impact if declined]: Crashes under certain timing conditions
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: All patches in this bug
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Well understood fix.
[String changes made/needed]: None

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9007363 [details]
Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell!

No crashes since it landed, uplift approved for 63 beta 7, thanks.

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8344af6b480d
https://hg.mozilla.org/releases/mozilla-beta/rev/d9b447bdf5e9
https://hg.mozilla.org/releases/mozilla-beta/rev/e7f051b7650e
https://hg.mozilla.org/releases/mozilla-beta/rev/fe879d96c14b