1221786 - about:webrtc includes calls from private browsing mode even after last pb session is closed

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

With the repositioning of private browsing mode, as mentioned in bug 1209252 comment 3, the fact that about:webrtc includes calls from private browsing mode even after the last private browsing session has been closed, should probably be fixed.

Options may be to:
1. trim or clear list once last private browsing session is closed.
2. never list private browsing calls in about:webrtc, or
3. Have a separate about:webrtc in private browsing mode.

Comment 1

[Maire Reavy [:mreavy]]

Paul -- Can you do the work for this when you do the other P1 bugs involving about:webrtc?  

As jib mentions in the description, we have a few options for how to solve this.  I think it makes sense to reach out to either bsmedberg or dveditz or both, lay out the options and decide what is the best behavior in private browsing mode.  I'm happy to help kick off the conversation with them if that would be helpful.

Comment 2

[Paul Kerr [:pkerr]]

Attachment: clear about:webrtc logs for private browsing sessions

No WebRTC session statistics will be saved after a PeerConnection
in a private browsing window is ended. In addition, the shared
WebRTC signalling log for that e10s process is cleared.

Comment 3

[Paul Kerr [:pkerr]]

While currently the most visible place to view the details of past WebRTC sessions run from a Private Browsing window, the about:webrtc page is not the best place to add privacy respecting guards to this data. The about:webrtc page uses the WebrtcGlobalInformation interface to access the WebRTC session information and ICE signalling log to compile its display. As such, this information is available to any chrome based (the WEBIDL is marked chrome-only) script via the global DOM.

The data returned from the WebrtcGlobalInformation interface is stored in two places. The data returned by the getAllStats() method is maintained by instances of PeerConnectionCtx. While the data returned by the getLogging() method is maintained in the RLogRingBuffer component of the signaling library. Both PeerConnectionCtx and RLogRingBuffer are created as singletons. Under E10S operation "singleton" instances of each exist in both the Chrome and Content processes with WebrtcGlobalInformation combining the data from each process instance into one block.

The approach that I have taken with this patch is to allow live session information from a WebRTC connection started in a private browsing session to be accessible via WebrtcGlobalInformation and to be displayable via about:webrtc. The motivation is to allow a session to be debugged even in a private browsing window. Then, when the connection is terminated, the WebRTC statistics for that session are not stored and the signalling log is cleared.

Note, the storage mechanism used by PeerConnectionCtx stores data from each session as a separate entry and allows the private session to simply be dropped without effected the rest of the session history. However, the signaling log is a simple buffer of string entries that holds the last N bytes of data. Since signaling trace information from private session can be interleaved with other signaling at a line level of granularity, the entire log is cleared with all history lost.

As jib noted above, another approach would be to not allow collection of data from a private browsing window. This is possible but would actually require more code to be added to the PeerConnection and ICE logging than is needed to simply drop and flush at the end of a session.

Comment 4

[Paul Kerr [:pkerr]]

Comment on attachment 8694826 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

dveditz: Can you provide some feedback this patch and perhaps some overall guidance on how to handle this issue in general. As with the issue that has been brought up about WebRTC signaling, this information can contain non-public IP addresses from call participants.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8694826 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

Review of attachment 8694826 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2591,5 @@
>      mDataConnection = nullptr; // it may not go away until the runnables are dead
>    }
> +
> +  if (mWindow && IsPrivateBrowsing(mWindow)) {
> +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();

Isn't RLogRingBuffer global?

So if I close any private browsing window, it clears the log for everyone, even regular non-private sessions?

Comment 6

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Comment on attachment 8694826 [details] [diff] [review]
> clear about:webrtc logs for private browsing sessions
> 
> Review of attachment 8694826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +2591,5 @@
> >      mDataConnection = nullptr; // it may not go away until the runnables are dead
> >    }
> > +
> > +  if (mWindow && IsPrivateBrowsing(mWindow)) {
> > +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();
> 
> Isn't RLogRingBuffer global?
> 
> So if I close any private browsing window, it clears the log for everyone,
> even regular non-private sessions?

jib: you are correct. That is a current side-effect. I thought about grabbing a selector from the deque that acts as a buffer to mark where the private browsing data started to be collected, but the shared nature of the buffer means that log strings from non-private session could be interleaved in the same segment of the buffer. Those strings would also be lost if that segment was flushed. Plus, the complicated logic of determining whether the selector held actually was still valid at the end of the session due to the action of the RemoveOld() buffer length maintenance mechanism. The process global nature is a problem. Handling this in PeerConnectionImpl is easier because each instance is associated with a window from which the private browsing state can be tested.

Comment 7

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Comment on attachment 8694826 [details] [diff] [review]
> clear about:webrtc logs for private browsing sessions
> 
> Review of attachment 8694826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +2591,5 @@
> >      mDataConnection = nullptr; // it may not go away until the runnables are dead
> >    }
> > +
> > +  if (mWindow && IsPrivateBrowsing(mWindow)) {
> > +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();
> 
> Isn't RLogRingBuffer global?
> 
> So if I close any private browsing window, it clears the log for everyone,
> even regular non-private sessions?

Also, you will eventually lose the signaling log strings for a call once the log buffer fills enough to drop those string from the log buffer.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This is why I was leaning toward not gathering data in private browsing mode. Why is debugging webrtc in private browsing mode important?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I should add that I don't mind strongly which way we go, as long as dveditz is on board. The global clearing on pb close seems surprising to me, and a bit unfortunate because of that, but likely wont be a big deal in practice.

Comment 10

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> This is why I was leaning toward not gathering data in private browsing
> mode. Why is debugging webrtc in private browsing mode important?

Nothing is particularly important about debugging; it is less complex to implement. With other session records, such as history and the cache, they operate in private browsing mode but disappear after the window is closed. Emulating that with WebrtcGlobalInformation is less complex: not saving the session stats and flushing the log when closing, than propagating a control flag into PeerConnectionImpl and RLogRingBuffer to block data collection when running under a private browsing window.

Comment 11

[Paul Kerr [:pkerr]]

Attachment: clear about:webrtc logs for private browsing sessions

Use mutex in RLogRingBuffer.Clear method.

Comment 12

[Paul Kerr [:pkerr]]

Comment on attachment 8698655 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

dveditz: Can you provide some feedback this patch and perhaps some overall guidance on how to handle this issue in general. As with the issue that has been brought up about WebRTC signaling, this information can contain non-public IP addresses from call participants.

Comment 13

[Daniel Veditz [:dveditz]]

"In general" we try not to mix state between Private Browsing and non-private, and then it's easy. Lots of ways we do that from separate storage (e.g. disk vs memory as with LocalStorage) to shared memory storage with bits tagged whether they were from PB so they're easy not to share and to discard later.

Another approach would be to disable the data gathering in private windows. How often do average people need to look at this, or even know what they're looking at? If they're having reproducible connection troubles someone helping them debug could direct them to retry and log in a non-private mode.

Or you could delete it all when you're done, as you've done here. The downside is that the data is mixed and available during the call, but it's not available to web content (hopefully no add-on would expose this for "remote debugging") and our PB threat model assumes the PB windows are closed before the snooper comes along.

These are basically the options jib noted in comment 0 except in reverse order of preference, and #2 has to be "don't gather" rather than "don't display" since in theory some helpful add-on could be doing something with the data.

You've chosen my least preferred option, but it does the job. The other options would have required thinking about private browsing when the code and interfaces were written--it's hard to retrofit after--so that's a fine choice.

Comment 14

[Paul Kerr [:pkerr]]

Attachment: clear about:webrtc logs for private browsing sessions

Based on feedback from reviewers, this version now handles the shared transport signaling log (RLogRingBuffer instance) by suspending trace message collection when a PeerConnection instance is running in a Private Window. This still prevents data collection from non-private windows but it no longer simply deletes the entire ice signaling log. Session statistics are still managed, as in the previous patch, on a per connection basis taking advantage of the individual record sets that are collected.

Comment 15

[Paul Kerr [:pkerr]]

Comment on attachment 8712192 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

Review of attachment 8712192 [details] [diff] [review]:
-----------------------------------------------------------------

JIB: I made changes to do something less harsh than deleting the entire ice signaling log. The connection stats are still handled in the same manner as in the previous patch; simply not saving the stats collection at the end of the session.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8712192 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

Review of attachment 8712192 [details] [diff] [review]:
-----------------------------------------------------------------

r- for likely silently capturing in private browsing mode if it's the very first WebRTC session after startup. Looks good otherwise, just nits, but would like to see it again.

::: media/mtransport/rlogringbuffer.cpp
@@ +94,5 @@
>    instance = nullptr;
>  }
>  
> +  // As long as any PeerConnection exists in a Private Window the rlog messages will not
> +  // be saved in the RLogRingBuffer. This is necessary because the log_messages buffer

Maybe s/saved/stored/ ? Seems confusing otherwise.

@@ +102,5 @@
> +void RLogRingBuffer::EnterPrivateMode() {
> +  OffTheBooksMutexAutoLock lock(mutex_);
> +  auto count = disableCount_ + 1;
> +  if (count == 0) {
> +    return;

When would disableCount_ ever be 0xffffffff ? Seems like dead code.

@@ +105,5 @@
> +  if (count == 0) {
> +    return;
> +  }
> +
> +  if (disableCount_ == 0) {

The count variable seems superfluous. Why not if (disableCount++ == 0) ?

@@ +115,5 @@
> +
> +void RLogRingBuffer::ExitPrivateMode() {
> +  OffTheBooksMutexAutoLock lock(mutex_);
> +  if (disableCount_ == 0) {
> +    return;

This also seems superfluous. These counters better never get out of sync, or we seem to be toast beyond what can be fixed with a floor.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +374,5 @@
>  
>    return true;
>  }
>  
> +bool IsPrivateBrowsing(nsPIDOMWindow *aWindow)

Nit: space after * not before. Applies throughout.

@@ +380,5 @@
> +#if defined(MOZILLA_EXTERNAL_LINKAGE)
> +  return false;
> +#else
> +  if (!aWindow) {
> +    return false;

It seems IsPrivateBrowsing() is only ever called with a non-null aWindow, so this is dead code. Maybe remove or MOZ_ASSERT?

@@ +430,5 @@
> +    if (mWindow && IsPrivateBrowsing(mWindow)) {
> +      mPrivateMode = true;
> +      auto log = RLogRingBuffer::GetInstance();
> +      if (log) {
> +        log->EnterPrivateMode();

Doesn't this need to be CreateInstance()? Otherwise wont we get nullptr here this early if this is the first WebRTC session, and end up silently capturing even if it's in private browsing mode?

@@ +2709,5 @@
> +
> +  if (mPrivateMode) {
> +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();
> +    if (log) {
> +      log->ExitPrivateMode();

I'm not liking the conditional around decrementing the counter here, because if the branch is ever taken, then the counter would get out of sync, and we'd be in trouble.

Maybe use CreateInstance() here as well just to get rid of the if, or leave GetInstance but add a MOZ_ASSERT? Fewer things to go wrong that way, and easier to reason about.

@@ +2713,5 @@
> +      log->ExitPrivateMode();
> +    }
> +    mPrivateMode = false;
> +  } else {
> +    RecordLongtermICEStatistics();

RecordLongtermICEStatistics() is no longer called if MOZILLA_EXTERNAL_LINKAGE, whereas before it was. Intentional? Hopefully its absence wont hurt any unit-tests?

Comment 17

[Paul Kerr [:pkerr]]

The last two lines in the log, when the first session is in Private Mode, do have the URI of the requested PeerConnection. The rest of the stuff is from mtransport initialization. I am looking for a mechanism to prevent the lines with the URIs.

Comment 18

[Paul Kerr [:pkerr]]

>@@ +2709,5 @@
>> +
>> +  if (mPrivateMode) {
>> +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();
>> +    if (log) {
>> +      log->ExitPrivateMode();
>
>I'm not liking the conditional around decrementing the counter here, because if the branch is >ever taken, then the counter would get out of sync, and we'd be in trouble.
>Maybe use CreateInstance() here as well just to get rid of the if, or leave GetInstance but add a >MOZ_ASSERT? Fewer things to go wrong that way, and easier to reason about.


If the GetInstance() call returns null, then the RLogRingBuffer has been deleted. There is no need to keep the balance between Enter and Exit. I simply don't want to de-reference the null pointer.

Comment 19

[Paul Kerr [:pkerr]]

In order to guard against the RLogRingBuffer.disableCount_ overflow or underflow, I have added MOZ_ASSERTs in place of the conditional statements in RLogRingBuffer.EnterPrivateMode/ExitPrivateMode.

I fixed the leak window by moving the RLogBuffer.Create into the PeerConnectionImpl constructor.

Comment 20

[Paul Kerr [:pkerr]]

Attachment: clear about:webrtc logs for private browsing sessions

No WebRTC session statistics will be saved after a PeerConnection
in a private browsing window is ended. In addition, the shared
WebRTC signalling log for that e10s process is cleared.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8712932 [details] [diff] [review]
clear about:webrtc logs for private browsing sessions

Review of attachment 8712932 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm provided NR_reg_init is idempotent.

::: media/mtransport/rlogringbuffer.cpp
@@ +79,4 @@
>  RLogRingBuffer* RLogRingBuffer::CreateInstance() {
>    if (!instance) {
>      instance = new RLogRingBuffer;
> +    NR_reg_init(NR_REG_MODE_LOCAL);

You're still calling this in nricectx.cpp as well. Is it idempotent?

@@ +106,5 @@
> +  ++disableCount_;
> +  MOZ_ASSERT(disableCount_ != 0);
> +
> +  if (disableCount_ == 1) {
> +    AddMsg(std::string("LOGGING SUSPENDED: a connection is active in a Private Window ***"));

(Missed last time) I'm almost sure std:string's implicit constructor works with move semantics as well, so we should be able to do the nicer:

AddMsg("LOGGING SUSPENDED: a connection is active in a Private Window ***");

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +425,4 @@
>  {
>  #if !defined(MOZILLA_EXTERNAL_LINKAGE)
>    MOZ_ASSERT(NS_IsMainThread());
> +  auto log = RLogRingBuffer::CreateInstance();

Can we use auto* at least, so readers know it can't be leaking (returning already_AddRefed)?

@@ +458,5 @@
>    // This aborts if not on main thread (in Debug builds)
>    PC_AUTO_ENTER_API_CALL_NO_CHECK();
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +  if (mPrivateWindow) {
> +    RLogRingBuffer* log = RLogRingBuffer::GetInstance();

Maybe pick RLogRingBuffer* or auto* in both places for consistency?

Comment 22

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #21)
> Comment on attachment 8712932 [details] [diff] [review]
> clear about:webrtc logs for private browsing sessions
> 
> Review of attachment 8712932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm provided NR_reg_init is idempotent.
> 
> ::: media/mtransport/rlogringbuffer.cpp
> @@ +79,4 @@
> >  RLogRingBuffer* RLogRingBuffer::CreateInstance() {
> >    if (!instance) {
> >      instance = new RLogRingBuffer;
> > +    NR_reg_init(NR_REG_MODE_LOCAL);
> 
> You're still calling this in nricectx.cpp as well. Is it idempotent?
> 
Yes, this call is idempotent. I left it in nricectx.cpp so I wouldn't break any stand-alone library tests.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3d939678e8

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d3d939678e8