1816708 - Assertion failure: mForceDropFrames != (PrincipalPrivacy::Private == mPrivacy), at /dom/media/webrtc/transportbridge/MediaPipeline.cpp:1500

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev 36b67e826e2d (built with: --enable-debug --enable-fuzzing).

This crash has been reported 1500 times since 2023/02/09. Marking as a fuzzblocker. Please prioritize accordingly.

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 36b67e826e2d --debug --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

Assertion failure: mForceDropFrames != (PrincipalPrivacy::Private == mPrivacy), at /dom/media/webrtc/transportbridge/MediaPipeline.cpp:1500

    ==17706==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4e16229ba1 bp 0x7fffabcc2440 sp 0x7fffabcc2420 T17706)
    ==17706==The signal is caused by a WRITE memory access.
    ==17706==Hint: address points to the zero page.
        #0 0x7f4e16229ba1 in SetPrivatePrincipal /dom/media/webrtc/transportbridge/MediaPipeline.cpp:1499:5
        #1 0x7f4e16229ba1 in mozilla::MediaPipelineReceiveVideo::SetPrivatePrincipal(nsMainThreadPtrHandle<nsIPrincipal>) /dom/media/webrtc/transportbridge/MediaPipeline.cpp:1643:16
        #2 0x7f4e160c5d12 in UpdatePrincipalPrivacy /dom/media/webrtc/jsapi/RTCRtpReceiver.cpp:827:14
        #3 0x7f4e160c5d12 in mozilla::dom::RTCRtpTransceiver::UpdatePrincipalPrivacy(mozilla::PrincipalPrivacy) /dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp:410:14
        #4 0x7f4e160ba274 in mozilla::PeerConnectionImpl::UpdateMediaPipelines() /dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:3715:18
        #5 0x7f4e16125fa5 in operator() /dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:2693:15
        #6 0x7f4e16125fa5 in mozilla::detail::RunnableFunction<mozilla::PeerConnectionImpl::DoSetDescriptionSuccessPostProcessing(mozilla::dom::RTCSdpType, bool, RefPtr<mozilla::dom::Promise> const&)::$_88>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:546:5
        #7 0x7f4e11cdfd65 in mozilla::RunnableTask::Run() /xpcom/threads/TaskController.cpp:539:16
        #8 0x7f4e11cdafac in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:852:26
        #9 0x7f4e11cd9b7a in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:684:15
        #10 0x7f4e11cd9ed5 in mozilla::TaskController::ProcessPendingMTTask(bool) /xpcom/threads/TaskController.cpp:462:36
        #11 0x7f4e11ce3816 in operator() /xpcom/threads/TaskController.cpp:188:37
        #12 0x7f4e11ce3816 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_2>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:546:5
        #13 0x7f4e11cf9107 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1225:16
        #14 0x7f4e11cff58d in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:477:10
        #15 0x7f4e1293c6e3 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:85:21
        #16 0x7f4e1285e618 in MessageLoop::RunInternal() /ipc/chromium/src/base/message_loop.cc:381:10
        #17 0x7f4e1285e521 in RunHandler /ipc/chromium/src/base/message_loop.cc:374:3
        #18 0x7f4e1285e521 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:356:3
        #19 0x7f4e16feeae8 in nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:148:27
        #20 0x7f4e1925a57b in XRE_RunAppShell() /toolkit/xre/nsEmbedFunctions.cpp:742:20
        #21 0x7f4e1293d5a9 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:235:9
        #22 0x7f4e1285e618 in MessageLoop::RunInternal() /ipc/chromium/src/base/message_loop.cc:381:10
        #23 0x7f4e1285e521 in RunHandler /ipc/chromium/src/base/message_loop.cc:374:3
        #24 0x7f4e1285e521 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:356:3
        #25 0x7f4e1925a0d8 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:675:34
        #26 0x5622d254cca0 in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #27 0x5622d254cca0 in main /browser/app/nsBrowserApp.cpp:353:18
        #28 0x7f4e25680d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #29 0x7f4e25680e3f in __libc_start_main csu/../csu/libc-start.c:392:3
        #30 0x5622d2523308 in _start (/home/jkratzer/builds/m-c-20230213170842-fuzzing-debug/firefox-bin+0x5b308) (BuildId: e6e538411639defba1d9fd0550053bfcdb425e0d)
    
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /dom/media/webrtc/transportbridge/MediaPipeline.cpp:1499:5 in SetPrivatePrincipal
    ==17706==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 2

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230214161440-e027953e2470.
The bug appears to have been introduced in the following build range:

Start: 6d37ee664b922e926a686feba165b20a628626e3 (20230209160701)
End: 640f3b3f1549d7b52354f1c0a5b133e41effd96f (20230209163152)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6d37ee664b922e926a686feba165b20a628626e3&tochange=640f3b3f1549d7b52354f1c0a5b133e41effd96f

Comment 3

[Mayank Bansal]

Got a crash from the testcase: https://crash-stats.mozilla.org/report/index/00591d6f-e1f8-45e4-98c7-d66840230215

Comment 4

[Edgar Chen [:edgar]]

(WebRTC: Audio/Video seems a proper componement)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 6

[Dianna Smith [:diannaS]]

apehrsons: from the regression range, this seems to have been caused by bug 1813468, could you take a look?

Comment 7

[Andreas Pehrson [:pehrsons]]

https://pernos.co/debug/slvkPktSEfMGbOe7ZgMLQg/index.html

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1816708 - Add crashtest. r?bwc!

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1816708 - Remove assertion assuming a non-private->private principal is the result of a network privacy request. r?bwc!

This assumption was bad, as the testcase shows it can come from js after
construction too, by negotiating a receive track with privacy when already
having a transceiver.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #2, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:pehrsons, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1813468

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b3b54dc8fb7
Add crashtest. r=bwc
https://hg.mozilla.org/integration/autoland/rev/0dd813c73faa
Remove assertion assuming a non-private->private principal is the result of a network privacy request. r=bwc

Comment 13

[Cristian Tuns]

Backed out for causing reftest failures on 1816708.html

• Backout link
• Push with failures
• Failure Log
• Failure line: REFTEST TEST-UNEXPECTED-FAIL | dom/media/webrtc/tests/crashtests/1816708.html | load failed: timed out waiting for reftest-wait to be removed

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1816708 - Allow insecure getUserMedia in webrtc crashtests. r?#webrtc-reviewers

These tests run over HTTP on Android.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

This bug prevents fuzzing from making progress; however, it has low severity. It is important for fuzz blocker bugs to be addressed in a timely manner (see here why?).
:pehrsons, could you increase the severity?

For more information, please visit auto_nag documentation.

Comment 16

[Andreas Pehrson [:pehrsons]]

Severity docs say that we use severity to "indicate the scope of a bug’s effect on Firefox". Fuzzing is not Firefox, so is the autonag message wrong, or are the severity docs wrong?

Should autonag nag about priority instead? Priority is already P1.

Comment 17

[Suhaib Mujahid [:suhaib]]

(In reply to Andreas Pehrson [:pehrsons] from comment #16)

Severity docs say that we use severity to "indicate the scope of a bug’s effect on Firefox". Fuzzing is not Firefox, so is the autonag message wrong, or are the severity docs wrong?

A fuzz blocker could prevent us from finding critical issues that are impacting Firefox users (e.g., security variabilities). Also, it increases the cost of the fuzzing. Thus, even when a bug seems trivial, its impact could be severe.

Should autonag nag about priority instead? Priority is already P1.

The severity of the issue is an input for the priority of the bug. It is excellent that the priority is already P1; however, by definition, the severity should indicate the scope of the bug impact.

Comment 18

[Andreas Pehrson [:pehrsons]]

(In reply to Suhaib Mujahid [:suhaib] from comment #17)

(In reply to Andreas Pehrson [:pehrsons] from comment #16)

Severity docs say that we use severity to "indicate the scope of a bug’s effect on Firefox". Fuzzing is not Firefox, so is the autonag message wrong, or are the severity docs wrong?

A fuzz blocker could prevent us from finding critical issues that are impacting Firefox users (e.g., security variabilities). Also, it increases the cost of the fuzzing. Thus, even when a bug seems trivial, its impact could be severe.

If we are setting severity based on what we might not be detecting then every bug that hasn't been fully investigated should be an S1, no?
Surely an engineer is better suited to estimate a bug's impact on the firefox product than autonag?

Should autonag nag about priority instead? Priority is already P1.

The severity of the issue is an input for the priority of the bug. It is excellent that the priority is already P1; however, by definition, the severity should indicate the scope of the bug impact.

In my mind the fact that it's a fuzz blocker is the input that makes it a P1 (and by extension, "to be addressed in a timely manner"). No need to increase the severity for that.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/581ebe8505cd
Allow insecure getUserMedia in webrtc crashtests. r=webrtc-reviewers,mjf
https://hg.mozilla.org/integration/autoland/rev/67bddedf8f88
Add crashtest. r=bwc
https://hg.mozilla.org/integration/autoland/rev/fbf003c7f0d7
Remove assertion assuming a non-private->private principal is the result of a network privacy request. r=bwc

Comment 20

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/581ebe8505cd
https://hg.mozilla.org/mozilla-central/rev/67bddedf8f88
https://hg.mozilla.org/mozilla-central/rev/fbf003c7f0d7

Comment 21

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9319158 [details]
Bug 1816708 - Remove assertion assuming a non-private->private principal is the result of a network privacy request. r?bwc!

Beta/Release Uplift Approval Request

• User impact if declined: Fuzzblocker
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): trivial, does not affect release
• String changes made/needed:
• Is Android affected?: Yes

Comment 22

[Andreas Pehrson [:pehrsons]]

Feel free to take the other patches too. They're test-only so shouldn't need approval.

Comment 23

[Donal Meehan [:dmeehan]]

Comment on attachment 9319158 [details]
Bug 1816708 - Remove assertion assuming a non-private->private principal is the result of a network privacy request. r?bwc!

Approved for 111.0b7

Comment 24

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bdc9564631d7
https://hg.mozilla.org/releases/mozilla-beta/rev/218df380ed47
https://hg.mozilla.org/releases/mozilla-beta/rev/311c7e3967c1

Comment 25

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230228085339-bc3bdd8c19f8.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 26

[Marco Castelluccio [:marco]]

(In reply to Andreas Pehrson [:pehrsons] from comment #18)

(In reply to Suhaib Mujahid [:suhaib] from comment #17)

(In reply to Andreas Pehrson [:pehrsons] from comment #16)

Severity docs say that we use severity to "indicate the scope of a bug’s effect on Firefox". Fuzzing is not Firefox, so is the autonag message wrong, or are the severity docs wrong?

A fuzz blocker could prevent us from finding critical issues that are impacting Firefox users (e.g., security variabilities). Also, it increases the cost of the fuzzing. Thus, even when a bug seems trivial, its impact could be severe.

If we are setting severity based on what we might not be detecting then every bug that hasn't been fully investigated should be an S1, no?

A fuzzblocker is guaranteed to be preventing fuzzing from working effectively, while other not yet investigated bugs might even be expected behavior or stuff like that.

Surely an engineer is better suited to estimate a bug's impact on the firefox product than autonag?

Definitely, the bot is merely suggesting an increase as it detects a potential discrepancy.

Based on the past experience of the fuzzing team regarding fuzzblockers and what can be found by resolving fuzzblockers, S4 doesn't usually fit with the risk (S4 is "minor significance, cosmetic issues, low or no impact to users").

It is the engineers' decision in the end to raise or not the severity.

Should autonag nag about priority instead? Priority is already P1.

The severity of the issue is an input for the priority of the bug. It is excellent that the priority is already P1; however, by definition, the severity should indicate the scope of the bug impact.

In my mind the fact that it's a fuzz blocker is the input that makes it a P1 (and by extension, "to be addressed in a timely manner"). No need to increase the severity for that.

I totally understand your reasoning, but unfortunately priority is not used by all teams, and it is used inconsistently by the teams that use it. We can't rely on it or touch it in the bot.

Comment 27

[Andreas Pehrson [:pehrsons]]

(In reply to Marco Castelluccio [:marco] from comment #26)

(In reply to Andreas Pehrson [:pehrsons] from comment #18)

(In reply to Suhaib Mujahid [:suhaib] from comment #17)

(In reply to Andreas Pehrson [:pehrsons] from comment #16)

Severity docs say that we use severity to "indicate the scope of a bug’s effect on Firefox". Fuzzing is not Firefox, so is the autonag message wrong, or are the severity docs wrong?

A fuzz blocker could prevent us from finding critical issues that are impacting Firefox users (e.g., security variabilities). Also, it increases the cost of the fuzzing. Thus, even when a bug seems trivial, its impact could be severe.

If we are setting severity based on what we might not be detecting then every bug that hasn't been fully investigated should be an S1, no?

A fuzzblocker is guaranteed to be preventing fuzzing from working effectively, while other not yet investigated bugs might even be expected behavior or stuff like that.

Surely an engineer is better suited to estimate a bug's impact on the firefox product than autonag?

Definitely, the bot is merely suggesting an increase as it detects a potential discrepancy.

Based on the past experience of the fuzzing team regarding fuzzblockers and what can be found by resolving fuzzblockers, S4 doesn't usually fit with the risk (S4 is "minor significance, cosmetic issues, low or no impact to users").

It is the engineers' decision in the end to raise or not the severity.

Should autonag nag about priority instead? Priority is already P1.

The severity of the issue is an input for the priority of the bug. It is excellent that the priority is already P1; however, by definition, the severity should indicate the scope of the bug impact.

In my mind the fact that it's a fuzz blocker is the input that makes it a P1 (and by extension, "to be addressed in a timely manner"). No need to increase the severity for that.

I totally understand your reasoning, but unfortunately priority is not used by all teams, and it is used inconsistently by the teams that use it. We can't rely on it or touch it in the bot.

Ok, I understand your position and I think you should clarify the docs on severity to include fuzzblockers, so you can back this line of argumentation up. Right now they say severity is an indicator of "the scope of a bug’s effect on Firefox", and the effect from the assert I'm removing here on Firefox in release is zero.

Also perhaps rework the autonag message because right now it doesn't sound like it's the engineer's decision much:
:pehrsons, could you increase the severity?

I for one would be happier with :pehrsons, could you consider increasing the severity?.

Comment 28

[Marco Castelluccio [:marco]]

I filed bug 1823813 to improve the docs and https://github.com/mozilla/relman-auto-nag/issues/1945 to improve the message. Thanks for the suggestions!