Crash in [@ mozilla::ipc::MessageChannel::~MessageChannel | mozilla::ipc::IToplevelProtocol::~IToplevelProtocol]
Categories
(Core :: Security: Process Sandboxing, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr102 | 112+ | fixed |
firefox110 | --- | unaffected |
firefox111 | --- | fixed |
firefox112 | --- | fixed |
People
(Reporter: aosmond, Assigned: aosmond)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
Bug 1819410 - Ensure we shutdown the RemoteSandboxBrokerParent IPDL instance during launch failures.
(deleted),
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-release+
diannaS
:
approval-mozilla-esr102+
|
Details |
Crash report: https://crash-stats.mozilla.org/report/index/9f91dc58-0163-4f2c-b9f5-f75ff0230228
MOZ_CRASH Reason: MOZ_CRASH(MessageChannel destroyed without being closed (mChannelState == ChannelConnected).)
Top 10 frames of crashing thread:
0 xul.dll mozilla::ipc::MessageChannel::~MessageChannel ipc/glue/MessageChannel.cpp:510
1 xul.dll mozilla::ipc::IToplevelProtocol::~IToplevelProtocol ipc/glue/ProtocolUtils.h:393
1 xul.dll mozilla::PRemoteSandboxBrokerChild::~PRemoteSandboxBrokerChild ipc/ipdl/PRemoteSandboxBrokerParent.cpp:43
2 xul.dll mozilla::RemoteSandboxBrokerParent::~RemoteSandboxBrokerParent security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h:16
2 xul.dll mozilla::RemoteSandboxBroker::~RemoteSandboxBroker security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp:21
3 xul.dll mozilla::RemoteSandboxBroker::~RemoteSandboxBroker security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp:17
4 xul.dll mozilla::AbstractSandboxBroker::Release security/sandbox/win/src/sandboxbroker/sandboxBroker.h:30
4 xul.dll mozilla::RefPtrTraits<mozilla::AbstractSandboxBroker>::Release mfbt/RefPtr.h:50
4 xul.dll RefPtr<mozilla::AbstractSandboxBroker>::ConstRemovingRefPtrTraits<mozilla::AbstractSandboxBroker>::Release mfbt/RefPtr.h:381
4 xul.dll RefPtr<mozilla::AbstractSandboxBroker>::~RefPtr mfbt/RefPtr.h:81
Assignee | ||
Comment 1•2 years ago
|
||
I was not able to reduce the regression window further:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f45ac8766b61&tochange=e027953e2470621b104e21045bec0a568c2e93d7
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d561ece0b43b&tochange=f45ac8766b6167ecd4adc81490f8f9b927472200
But something subtly made us crash the GMP plugin process later than before.
Originally we would fail and hit this path:
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp#97
We would not have successfully setup the IPDL channel and so there was nothing to teardown.
Now we fail to send the initial IPC messages at startup because the process crashes just after:
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp#106
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp#114
Since the higher levels assume that the failure to launch the process means nothing is setup / it doesn't have to teardown our sandbox broker IPDL objects, we should do it here before returning.
Assignee | ||
Comment 2•2 years ago
|
||
STR: On Windows ARM with a clean profile and DRM enabled, go to https://shaka-player-demo.appspot.com/demo/. The parent process will crash.
Assignee | ||
Comment 3•2 years ago
|
||
If the process being launched by the sandbox broker crashes after we
setup the IPDL channel, but before we successfully complete the
initialization process, we don't explicitly teardown
RemoteSandboxBrokerParent. As such, when we destroy the owning objects,
it attempts to destroy RemoteSandboxBrokerParent while the protocol is
still open, triggering a release assert for destroying
PRemoteSandboxBrokerParent while the protocol/channel are still open.
This patch makes us explicitly teardown the channel if there is a launch
failure inside the sandbox broker.
Comment 5•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Assignee | ||
Comment 6•2 years ago
|
||
Comment on attachment 9320333 [details]
Bug 1819410 - Ensure we shutdown the RemoteSandboxBrokerParent IPDL instance during launch failures.
Beta/Release Uplift Approval Request
- User impact if declined: Any streaming site depending on Widevine and Clearkey plugins are broken with the x86 plugins on Windows ARM. Uplifting the requested set will allow us to roll out a switch to the ARM plugins instead.
Visiting any DRM protected streaming site will crash the plugin process, and the user will be unable to play content.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: Bug 1814910, Bug 1814703, Bug 1819661, Bug 1814703, Bug 1820669
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Low risk because we have verified this in 112 in both nightly and beta with no issues reported by QA. GMP plugins are already completely broken for Windows ARM users.
- String changes made/needed:
- Is Android affected?: Yes
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Any streaming site depending on Widevine and Clearkey plugins are broken with the x86 plugins on Windows ARM. Uplifting the requested set will allow us to roll out a switch to the ARM plugins instead.
- User impact if declined: Visiting any DRM protected streaming site will crash the plugin process, and the user will be unable to play content.
- Fix Landed on Version: 112
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Low risk because we have verified this in 112 in both nightly and beta with no issues reported by QA. GMP plugins are already completely broken for Windows ARM users.
Depends on:
Bug 1814910, Bug 1814703, Bug 1819661, Bug 1814703, Bug 1820669, Bug 1811981
Comment 7•2 years ago
|
||
Comment on attachment 9320333 [details]
Bug 1819410 - Ensure we shutdown the RemoteSandboxBrokerParent IPDL instance during launch failures.
Approved for 102.10esr
Comment 8•2 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Comment 9•2 years ago
|
||
Comment on attachment 9320333 [details]
Bug 1819410 - Ensure we shutdown the RemoteSandboxBrokerParent IPDL instance during launch failures.
Approved for 111.0.1
Updated•2 years ago
|
Comment 10•2 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Description
•