1717318 - Crash with failed "@mozilla.org/peerconnection;1" instance (round 2)

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Basically the same as bug 1550540. It regressed just some months after getting fixed.

Run this code in the browser console:

Cc["@mozilla.org/peerconnection;1"].createInstance(Ci.nsIArray)

Expected: it throws a JS exception like

[Exception... "ComponentManager::CreateInstance returned failure code:"  nsresult: "0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Result: Firefox crashes

AddressSanitizer:DEADLYSIGNAL

Regression window: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=49320c7fe8b3a547856dc67e7d57a64db6b56ffd&tochange=9e3f44e87a1a2be927c7fab135653c6b86b982c9

Comment 1

[Oriol Brufau [:Oriol]]

https://crash-stats.mozilla.org/report/index/a85a6b98-62d0-439b-af83-bbbae0210619
https://crash-stats.mozilla.org/report/index/a442a38f-2921-4c43-8b01-6a3a60210619

Basically,

NS_IMPL_COMPONENT_FACTORY(mozilla::PeerConnectionImpl) {
  return do_AddRef(new PeerConnectionImpl()).downcast<nsISupports>();
}

instantiates PeerConnectionImpl with a nullprt aGlobal.

But RTCStatsTimestampMaker::RTCStatsTimestampMaker assumes it's not null:

  nsCOMPtr<nsPIDOMWindowInner> window =
      do_QueryInterface(aGlobal->GetAsSupports());

So this was regressed by bug 1588588.

Comment 2

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4d7b7391cf9f1eedac5fe496442dda37253380
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a866db13a130df82a304cb4c896235844a5fe4e8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6029ce289e6372c7183a3964583557a2a4403490

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1717318: Test-case for bug. r?mjf

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1717318: Stop PeerConnectionImpl from crashing if constructed with a null global. r?mjf

Depends on D124061

Comment 5

[Byron Campen [:bwc]]

Mochitest and wpt look fine, not sure about crashtest though. Here's a baseline push for comparison:

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

Comment 6

[Byron Campen [:bwc]]

Yeah, try looks the same as baseline.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5c54bb33074
Test-case for bug. r=mjf
https://hg.mozilla.org/integration/autoland/rev/d755daa2871c
Stop PeerConnectionImpl from crashing if constructed with a null global. r=mjf

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b5c54bb33074
https://hg.mozilla.org/mozilla-central/rev/d755daa2871c

Comment 9

[Michael Froman [:mjf]]

Comment on attachment 9238687 [details]
Bug 1717318: Stop PeerConnectionImpl from crashing if constructed with a null global. r?mjf

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a simple fix checking for a null ptr to avoid a crash.
• User impact if declined: Possible crash in the wild.
• Fix Landed on Version: 93
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small number of changes (2 asserts and a null deref check).
• String or UUID changes made by this patch: n/a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9238686 [details]
Bug 1717318: Test-case for bug. r?mjf

Approved for 91.4esr, thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/2c9565d45c47
https://hg.mozilla.org/releases/mozilla-esr91/rev/4b0d941ab1a3