Closed Bug 1489317 Opened Last year Closed Last year

Crash in OXIDEntry::StartServer

Categories

(Core :: Widget: Win32, defect, critical)

63 Branch
All
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: philipp, Assigned: aklotz)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(4 files)

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.
This looks like fallout from bug 1425144.
Blocks: 1425144
I'll take this, as it actually requires some mscom magic.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
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.
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 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.
Attachment #9007365 - Flags: review+
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.
Attachment #9007363 - Flags: review+
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.
Attachment #9007359 - Flags: review+
Comment on attachment 9007360 [details]
Bug 1489317: Part 2 - Improvements to mscom::AgileReference; r=froydnj!

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9007360 - Flags: review+
Pushed by aklotz@mozilla.com:
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
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.
Pushed by aklotz@mozilla.com:
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
Pushed by apavel@mozilla.com:
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
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.
Flags: needinfo?(aklotz)
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/151b11e40af6
Backed out 4 changesets for causing multiple windows tests timeouts. CLOSED TREE
(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...
Flags: needinfo?(aklotz)
Attachment #9007359 - Attachment description: Bug 1489317: Part 1 - Add forward delcaration for mscom::AgileReference to RefPtr; r=froydnj! → Bug 1489317: Part 1 - Add forward declaration for mscom::AgileReference to RefPtr; r=froydnj!
Pushed by aklotz@mozilla.com:
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
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
Flags: needinfo?(aklotz)
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
Flags: needinfo?(aklotz)
Attachment #9007363 - Flags: approval-mozilla-beta?
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.
Attachment #9007363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.