Closed Bug 1838413 Opened 2 years ago Closed 2 years ago

Intermittent /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | single tracking bug

Categories

(Core :: WebRTC, defect, P5)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 --- unaffected
firefox114 --- unaffected
firefox115 --- unaffected
firefox116 --- wontfix
firefox117 --- wontfix
firefox122 --- wontfix
firefox123 --- wontfix
firefox124 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: bwc)

References

(Regression)

Details

(Keywords: intermittent-failure, intermittent-testcase, regression, Whiteboard: [stockwell disable-recommended], [wptsync upstream])

Attachments

(1 file)

Bug 1838413: Make sure we hold a reference to the pc in these tests. r?pehrsons
48 bytes, text/x-phabricator-request
Details | Review

Filed by: nfay [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=419250059&repo=mozilla-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/EuL1GR8kTGmm-v90O-5qbg/runs/0/artifacts/public/logs/live_backing.log

[task 2023-06-14T11:03:33.759Z] 11:03:33     INFO - TEST-START | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html
[task 2023-06-14T11:03:33.800Z] 11:03:33     INFO - Closing window aa708515-6002-4642-a65d-45864b596181
[task 2023-06-14T11:03:33.936Z] 11:03:33     INFO - Closing window 195ba219-0592-42e0-b00f-c70401020413
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RTCTrackEvent-fire.html | Applying a remote description with a new msid should trigger firing an event with populated streams 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [audio] Track with stream: removal due to disassociation in rollback and then add it back again 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [audio] Track without stream: removal due to disassociation in rollback and then add it back 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [audio] Track with stream: removal due to direction changing and then add back using rollback 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [audio] Track without stream: removal due to direction changing and then add back using rollback 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [video] Track with stream: removal due to disassociation in rollback and then add it back again 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [video] Track without stream: removal due to disassociation in rollback and then add it back 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [video] Track with stream: removal due to direction changing and then add back using rollback 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/RollbackEvents.https.html | [video] Track without stream: removal due to direction changing and then add back using rollback 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-PASS | /webrtc/back-forward-cache-with-closed-webrtc-connection.https.window.html | Testing BFCache support for page with closed WebRTC connection. 
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - TEST-UNEXPECTED-FAIL | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | Testing BFCache support for page with open WebRTC connection. - assert_equals: expected (undefined) undefined but got (boolean) true
[task 2023-06-14T11:03:36.648Z] 11:03:36     INFO - assertNotRestoredFromBFCache@https://web-platform.test:8443/html/browsers/browsing-the-web/back-forward-cache/resources/rc-helper.js:43:16
[task 2023-06-14T11:03:36.725Z] 11:03:36     INFO - TEST-OK | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | took 2965ms
[task 2023-06-14T11:03:37.175Z] 11:03:37     INFO - STDOUT: cleanup aborted: Unable to remount device
[task 2023-06-14T11:03:37.284Z] 11:03:37     INFO - STDOUT: cleanup aborted: Unable to remount device
[task 2023-06-14T11:03:37.285Z] 11:03:37     INFO - Closing logging queue
[task 2023-06-14T11:03:37.285Z] 11:03:37     INFO - queue closed
[task 2023-06-14T11:03:37.299Z] 11:03:37     INFO - Setting up ssl
[task 2023-06-14T11:03:37.320Z] 11:03:37     INFO - certutil | b''
[task 2023-06-14T11:03:37.338Z] 11:03:37     INFO - certutil | b''
[task 2023-06-14T11:03:37.350Z] 11:03:37     INFO - certutil | b'\nCertificate Nickname                                         Trust Attributes\n                                                             SSL,S/MIME,JAR/XPI\n\nweb-platform-tests                                           CT,, \n'
[task 2023-06-14T11:03:38.055Z] 11:03:38     INFO - adb Granting important runtime permissions to org.mozilla.geckoview.test_runner
[task 2023-06-14T11:03:39.402Z] 11:03:39     INFO - adb launch_application: am start -W -n org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.TestRunnerActivity -a android.intent.action.MAIN --es env0 MOZ_CRASHREPORTER=1 --es env1 MOZ_CRASHREPORTER_NO_REPORT=1 --es env2 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env3 MOZ_HIDE_RESULTS_TABLE=1 --es env4 MOZ_IN_AUTOMATION=1 --es env5 MOZ_LOG=signaling:3,mtransport:4,DataChannel:4,jsep:4 --es env6 R_LOG_LEVEL=6 --es env7 R_LOG_DESTINATION=stderr --es env8 R_LOG_VERBOSE=1 --es env9 MOZ_PROCESS_LOG=/tmp/tmpxa2d97d5pidlog --es env10 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es arg0 -no-remote --es arg1 -profile --es arg2 /data/local/tmp/test_root/profile --es arg3 --marionette --es arg4 about:blank --ez use_multiprocess True
[task 2023-06-14T11:03:40.762Z] 11:03:40     INFO - Starting runner
[task 2023-06-14T11:03:43.887Z] 11:03:43     INFO - TEST-START | /webrtc/getstats.html
Keywords: regression
Regressed by: 1834254

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

status-firefox114: --- → unaffected
status-firefox115: --- → unaffected
status-firefox116: --- → affected
status-firefox-esr102: --- → unaffected

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

status-firefox117: --- → affected
status-firefox-esr115: --- → unaffected

https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
For more information, please visit BugBot documentation.

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox116: affected → wontfix
status-firefox117: affected → wontfix
Resolution: --- → INCOMPLETE

New failure instance: https://treeherder.mozilla.org/logviewer?job_id=441023164&repo=autoland

Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---

https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
For more information, please visit BugBot documentation.

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
Resolution: --- → INCOMPLETE

New failure instance: https://treeherder.mozilla.org/logviewer?job_id=444477373&repo=autoland

Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---

The rate on this has gone way up starting on the 24th. Trying to figure out what caused this while it is fresh.

Ok, looks like this is due to a modification to the test:

https://hg.mozilla.org/mozilla-central/rev/5da45fef761441269636bc9268a143e1944a5b25

Any ideas here?

Flags: needinfo?(apehrson)

Update

There have been 101 failures since the bug has been reopened.

  • 1 failure on Android 7.0 x86-64 Lite WebRender opt
  • 7 failures on Android 7.0 x86-64 WebRender opt
  • 4 failures on Linux 18.04 WebRender Shippable opt
  • 48 failures on Linux 18.04 x64 WebRender asan opt
  • 20 failures on Linux 18.04 x64 WebRender debug/ opt
  • 1 failure on Linux 18.04 x64 WebRender Shippable opt
  • 7 failures on OS X 10.15 WebRender debug/opt
  • 2 failures on OS X 10.15 WebRender Shippable opt
  • 3 failures on Windows 11 x86 22H2 WebRender debug
  • 1 failure on Windows 11 x64 22H2 CCov WebRender opt
  • 5 failures on Windows 11 x64 22H2 WebRender debug
  • 2 failures on Windows 11 x64 22H2 WebRender Shippable opt

Recent log: https://treeherder.mozilla.org/logviewer?job_id=444954825&repo=mozilla-central&lineNumber=9174

Byron, could we have this disabled at least on the platform with most occurrences, until we'll get a reply from Andreas?
Thank you.

Flags: needinfo?(docfaraday)
Whiteboard: [stockwell needswork:owner]

nsIDocumentActivity seems to be the generic way one gets notifications about a document becoming inactive (like entering bfcache). There's nothing generic for MediaStreamTracks. MediaRecorder has some handling, and there's a special case for HTMLMediaElement. How do we handle this with RTCPeerConnection? In js?

Flags: needinfo?(apehrson)

(In reply to Byron Campen [:bwc] from comment #25)

Ok, looks like this is due to a modification to the test:

https://hg.mozilla.org/mozilla-central/rev/5da45fef761441269636bc9268a143e1944a5b25

How certain are you this particular commit is the regressor? Because we don't implement the NotRestoredReason API so those strings are ignored.

It looks super likely to be the case; we're hitting a failure in the test-case that commit adds. The link in comment 28 has a line number referring to an unrelated failure that also occurred, we need to be looking at this: https://treeherder.mozilla.org/logviewer?job_id=444954825&repo=mozilla-central&lineNumber=118039

Flags: needinfo?(docfaraday)

(In reply to Natalia Csoregi [:nataliaCs] from comment #28)

Update

There have been 101 failures since the bug has been reopened.

  • 1 failure on Android 7.0 x86-64 Lite WebRender opt
  • 7 failures on Android 7.0 x86-64 WebRender opt
  • 4 failures on Linux 18.04 WebRender Shippable opt
  • 48 failures on Linux 18.04 x64 WebRender asan opt
  • 20 failures on Linux 18.04 x64 WebRender debug/ opt
  • 1 failure on Linux 18.04 x64 WebRender Shippable opt
  • 7 failures on OS X 10.15 WebRender debug/opt
  • 2 failures on OS X 10.15 WebRender Shippable opt
  • 3 failures on Windows 11 x86 22H2 WebRender debug
  • 1 failure on Windows 11 x64 22H2 CCov WebRender opt
  • 5 failures on Windows 11 x64 22H2 WebRender debug
  • 2 failures on Windows 11 x64 22H2 WebRender Shippable opt

Recent log: https://treeherder.mozilla.org/logviewer?job_id=444954825&repo=mozilla-central&lineNumber=9174

Byron, could we have this disabled at least on the platform with most occurrences, until we'll get a reply from Andreas?
Thank you.

I think it would be fine to mark this test as intermittent, if wpt-sync-bot has not already done so.

(In reply to Byron Campen [:bwc] from comment #33)

It looks super likely to be the case; we're hitting a failure in the test-case that commit adds. The link in comment 28 has a line number referring to an unrelated failure that also occurred, we need to be looking at this: https://treeherder.mozilla.org/logviewer?job_id=444954825&repo=mozilla-central&lineNumber=118039

My point is the commit https://hg.mozilla.org/mozilla-central/rev/5da45fef761441269636bc9268a143e1944a5b25 does not add a test case.

I think I see the problem in the log as it logs the Timecard:

[task 2024-01-28T22:36:44.085Z] 22:36:44     INFO - TEST-START | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html
(...)
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - TEST-UNEXPECTED-FAIL | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | Testing BFCache support for page with open WebRTC connection and live MediaStreamTrack. - assert_equals: document unexpectedly BFCached expected (undefined) undefined but got (boolean) true
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - assertNotRestoredFromBFCache@https://web-platform.test:8443/html/browsers/browsing-the-web/back-forward-cache/resources/rc-helper.js:82:16
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - TEST-OK | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | took 4843ms
(...)
[task 2024-01-28T22:36:48.996Z] 22:36:48     INFO - PID 3305 | Timecard created 1706481404.466482
[task 2024-01-28T22:36:48.997Z] 22:36:48     INFO - PID 3305 |  Timestamp   | Delta       | Event                                  | File                         | Function
[task 2024-01-28T22:36:48.998Z] 22:36:48     INFO - PID 3305 | ==========================================================================================================================
[task 2024-01-28T22:36:48.998Z] 22:36:48     INFO - PID 3305 |     0.000063 |    0.000063 | Constructor Completed                  | PeerConnectionImpl.cpp:388   | PeerConnectionImpl
[task 2024-01-28T22:36:48.999Z] 22:36:48     INFO - PID 3305 |     0.000533 |    0.000470 | Initializing PC Ctx                    | PeerConnectionImpl.cpp:473   | Initialize
[task 2024-01-28T22:36:48.999Z] 22:36:48     INFO - PID 3305 |     0.001602 |    0.001069 | Set Remote Description                 | PeerConnectionImpl.cpp:1765  | SetRemoteDescription
[task 2024-01-28T22:36:49.000Z] 22:36:49     INFO - PID 3305 |     0.008086 |    0.006484 | Add Ice Candidate                      | PeerConnectionImpl.cpp:1881  | AddIceCandidate
[task 2024-01-28T22:36:49.000Z] 22:36:49     INFO - PID 3305 |     0.427854 |    0.419768 | Close                                  | PeerConnectionImpl.cpp:2533  | Close
[task 2024-01-28T22:36:49.001Z] 22:36:49     INFO - PID 3305 |     4.529347 |    4.101493 | Destructor Invoked                     | PeerConnectionImpl.cpp:405   | ~PeerConnectionImpl
[task 2024-01-28T22:36:49.002Z] 22:36:49     INFO - PID 3305 |     4.529347 |    0.000000 | {876fb5fc-3f19-4c64-a217-087834c56669} | PeerConnectionImpl.cpp:406   | ~PeerConnectionImpl

PeerConnectionImpl::Close is what removes the bfcache block. What is calling Close? I guess the real question is, what should guarantee that a peer connection stays open here? I.e. spec or impl issue?

Flags: needinfo?(docfaraday)

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

(In reply to Byron Campen [:bwc] from comment #33)

It looks super likely to be the case; we're hitting a failure in the test-case that commit adds. The link in comment 28 has a line number referring to an unrelated failure that also occurred, we need to be looking at this: https://treeherder.mozilla.org/logviewer?job_id=444954825&repo=mozilla-central&lineNumber=118039

My point is the commit https://hg.mozilla.org/mozilla-central/rev/5da45fef761441269636bc9268a143e1944a5b25 does not add a test case.

Ah, it was probably the ini file changes that rode along:

https://hg.mozilla.org/mozilla-central/diff/398b82c2779fdfeb233c7115f57ce671a7cf3097/testing/web-platform/meta/webrtc/back-forward-cache-with-open-webrtc-connection.https.window.js.ini

This test was likely never working reliably.

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

I think I see the problem in the log as it logs the Timecard:

[task 2024-01-28T22:36:44.085Z] 22:36:44     INFO - TEST-START | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html
(...)
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - TEST-UNEXPECTED-FAIL | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | Testing BFCache support for page with open WebRTC connection and live MediaStreamTrack. - assert_equals: document unexpectedly BFCached expected (undefined) undefined but got (boolean) true
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - assertNotRestoredFromBFCache@https://web-platform.test:8443/html/browsers/browsing-the-web/back-forward-cache/resources/rc-helper.js:82:16
[task 2024-01-28T22:36:48.928Z] 22:36:48     INFO - TEST-OK | /webrtc/back-forward-cache-with-open-webrtc-connection.https.window.html | took 4843ms
(...)
[task 2024-01-28T22:36:48.996Z] 22:36:48     INFO - PID 3305 | Timecard created 1706481404.466482
[task 2024-01-28T22:36:48.997Z] 22:36:48     INFO - PID 3305 |  Timestamp   | Delta       | Event                                  | File                         | Function
[task 2024-01-28T22:36:48.998Z] 22:36:48     INFO - PID 3305 | ==========================================================================================================================
[task 2024-01-28T22:36:48.998Z] 22:36:48     INFO - PID 3305 |     0.000063 |    0.000063 | Constructor Completed                  | PeerConnectionImpl.cpp:388   | PeerConnectionImpl
[task 2024-01-28T22:36:48.999Z] 22:36:48     INFO - PID 3305 |     0.000533 |    0.000470 | Initializing PC Ctx                    | PeerConnectionImpl.cpp:473   | Initialize
[task 2024-01-28T22:36:48.999Z] 22:36:48     INFO - PID 3305 |     0.001602 |    0.001069 | Set Remote Description                 | PeerConnectionImpl.cpp:1765  | SetRemoteDescription
[task 2024-01-28T22:36:49.000Z] 22:36:49     INFO - PID 3305 |     0.008086 |    0.006484 | Add Ice Candidate                      | PeerConnectionImpl.cpp:1881  | AddIceCandidate
[task 2024-01-28T22:36:49.000Z] 22:36:49     INFO - PID 3305 |     0.427854 |    0.419768 | Close                                  | PeerConnectionImpl.cpp:2533  | Close
[task 2024-01-28T22:36:49.001Z] 22:36:49     INFO - PID 3305 |     4.529347 |    4.101493 | Destructor Invoked                     | PeerConnectionImpl.cpp:405   | ~PeerConnectionImpl
[task 2024-01-28T22:36:49.002Z] 22:36:49     INFO - PID 3305 |     4.529347 |    0.000000 | {876fb5fc-3f19-4c64-a217-087834c56669} | PeerConnectionImpl.cpp:406   | ~PeerConnectionImpl

PeerConnectionImpl::Close is what removes the bfcache block. What is calling Close? I guess the real question is, what should guarantee that a peer connection stays open here? I.e. spec or impl issue?

Close can be called either explicitly by JS, or implicitly in Unlink:

https://searchfox.org/mozilla-central/rev/3c72de9280ec57dc55c24886c6334d9e340500e8/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp#254

However, if Unlink is doing this, I would expect the d'tor to be invoked much sooner. That said, I'm not seeing anything that would keep the PC alive. We aren't registering any event handlers, or adding any tracks, or negotiating.

Flags: needinfo?(docfaraday)

So, even if I disable this call when Close is called from unlink, we still see intermittent failures:

https://searchfox.org/mozilla-central/source/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp#2580

Something is fishy here.

Ahha. Had one other callsite to modify.

So, the PC is getting unlinked because the page is going away. So it marks the window as bfcacheable on its way down, it seems. That seems wrong to me. I think we want to avoid doing this if we're unlinking because the page is going away, but not sure what the proper way to do this is.

When I modify PeerConnectionImpl to only clear the bfcache blocker if it is explicitly closed by JS, we pass all tests, which indicates that we're missing some tests here. If JS keeps the window open, but relinquishes all references to the RTCPeerConnection, I would expect the same behavior as JS calling close(). I wonder if other things (eg; WebSockets, WebTransport) might need some tightening up as well.

Looking at this more, we seem to be blocking bfcache in a different way than others. In most cases, we seem to be using WindowGlobalChild::BlockBFCacheFor

https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom17WindowGlobalChild15BlockBFCacheForENS0_13BFCacheStatusE&redirect=false

Maybe using this API instead will result in better behavior.

Edit: Using this API does not appear to make any difference.

Looking at this test some more, and this is weirder than it looked at first glance. We are reliably passing this check (assertBFCacheEligibility):

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webrtc/back-forward-cache-with-open-webrtc-connection.https.window.js#18

But frequently failing the next one (assertNotRestoredFromBFCache):

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webrtc/back-forward-cache-with-open-webrtc-connection.https.window.js#19

Because this assertion is seeing true instead of undefined:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/browsers/browsing-the-web/back-forward-cache/resources/rc-helper.js#82

However, assertBFCacheEligibility (which passes reliably) calls assertNotRestoredFromBFCache (the check that fails!); this is exactly the same check that is causing intermittent failures, and both calls make exactly the same assertion:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/browsers/browsing-the-web/back-forward-cache/resources/rc-helper.js#125-126,135

So, it seems that after navigating back, at first we see window.beforeBFCache == undefined, but then window.beforeBFCache sometimes becomes true without any additional navigation taking place? What is up with that?

Ok, it seems that I just got really unlucky, and never had a failure when I removed the stand-alone call to assertNotRestoredFromBFCache.

Ok, so this really does seem to be a case where the PC is being cycle collected too early for the test to work. I'm not totally sure what the spec says ought to happen in this test, but we aren't holding that pc reference anywhere, we aren't registering any event handlers, and we aren't trying to connect or start ICE. Storing a ref to the pc on the window is enough to get this test passing:

https://treeherder.mozilla.org/jobs?repo=try&revision=27b7fd4be61b530b2761f079cd234f0c697b98cf

Flags: needinfo?(peterv)

It is not super clear what the rules are for whether a PeerConnection is considered active enough to warrant blocking bfcache. We block bfcache regardless of how much the PeerConnection has been used, but the author of the test is performing a setRemoteDescription and an addIceCandidate, presumably to make Chrome's implementation "active" enough to cause them to block bfcache?

Assignee: nobody → docfaraday
See Also: → 1879843

I had started looking at this, but I couldn't figure out what would be keeping the RTCPeerConnection alive if it's not stored somewhere but not closed either. https://w3c.github.io/webrtc-pc/#garbage-collection does mention checking [[IsClosed]] before garbage collecting. RTCPeerConnection.close() definitely sets that to true (through close the connection). https://w3c.github.io/webrtc-pc/#operation does add something to unloading document cleanup steps which also calls close the connection.

Does the spec language mean that as long as a RTCPeerConnection hasn't been closed (so [[IsClosed]] is false) we might need to keep it alive? In particular, I'm going to need some help here to understand what MUST not be garbage collected as long as any event can cause an event handler to be triggered on the object means. Does that mean it can only be GCed after close the connection has been called? We'll probably need to use some tricks with cycle collection where the object keeps itself alive by adding a reference from the global or something to mimic that.

(Edited comment because I got confused about BFCaching with open RTCPeerConnections)

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

nsIDocumentActivity seems to be the generic way one gets notifications about a document becoming inactive (like entering bfcache).

If we do need to keep the RTCPeerConnection object alive until we're in unloading document cleanup steps then registering as an activity observer could work (and keeping an artificial reference to the object until then), but it's not available to script yet. I don't think we have a great place in our code yet for exactly that part of the unloading algorithms.

Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c4a0321d30a Make sure we hold a reference to the pc in these tests. r=pehrsons
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44538 for changes under testing/web-platform/tests
Whiteboard: [stockwell disable-recommended] → [stockwell disable-recommended], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/7c4a0321d30a

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox124: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch
Upstream PR merged by moz-wptsync-bot

(In reply to Peter Van der Beken [:peterv] from comment #52)

Does the spec language mean that as long as a RTCPeerConnection hasn't been closed (so [[IsClosed]] is false) we might need to keep it alive?

As far as intent goes, I don't think so, or it could have said so more simply I think (but it might allude to that being an acceptable implementation, casting a wide net).

In particular, I'm going to need some help here to understand what MUST not be garbage collected as long as any event can cause an event handler to be triggered on the object means. Does that mean it can only be GCed after close the connection has been called?

I'll note it

  • DOESN'T say "An RTCPeerConnection object MUST not be garbage collected as long as any event can be fired on the object."
  • it DOES say "An RTCPeerConnection object MUST not be garbage collected as long as any event can cause an event handler to be triggered on the object."

This seems to imply it matters whether event handlers are registered. But looking at this again, the spec language might need improving.

Having been somewhat involved with writing this, I recall this being modeled after https://websockets.spec.whatwg.org/#garbage-collection, which was the closest relevant precedent we had to look at at that point (that dealt with a remote end-point).

I should say my interpretation of spec-specific GC prose in general is as attempts to describe when GCing a complex object cannot be observed, where this isn't obvious due to the complexity of that particular object.

It is NOT my interpretation that these statements are allowances for JS to observe GC. Most of the prose like this was written back when observing GC was an absolute no-no per https://w3ctag.github.io/design-principles/#js-gc, and predate WeakRef.

My interpretation of the WebSocket note is as an attempt to describe when GCing the WebSocket object cannot be observed locally AND when a remote end-point is guaranteed no more input can come from this side. To that end, I think it succeeds, because it covers all the ways WebSocket state and IO (with input only receivable trough events in this case) can be observed.

The flaw with WebRTC copying this model may be that WebRTC is more complicated than WebSocket, so it's harder to write down all the cases. E.g. video streams can be consumed by e.g. video.srcObject = rtcRtpTransceiver.receiver.track but as long as this requires strong references we should be fine.

All that to say:

We'll probably need to use some tricks with cycle collection where the object keeps itself alive by adding a reference from the global or something to mimic that.

I think I'd rather we fix the spec than resort to tricks in the cycle collector. Looking at this particular test, do we think this can happen outside of browser-tests?

Flags: needinfo?(docfaraday)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #58)

I think I'd rather we fix the spec than resort to tricks in the cycle collector. Looking at this particular test, do we think this can happen outside of browser-tests?

What's "this" referring to here? An RTCPeerConnection getting collected earlier than we want?

Flags: needinfo?(docfaraday)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: