874061 - Figure out private browsing and the browser console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Mihai Sucan [:msucan]]

Private browsing behavior is undefined in the Browser and Web Consoles.

Overview of the current state:

- the DOM ConsoleAPI.js checks if the message comes from a private window. If yes, the console message is not stored in the console API cache.
- the nsIScriptError has a private mode flag and messages end-up in the cache.
- the Error Console skips all nsIScriptErrors that come from private windows, even if they are chrome messages.
- the web console shows all nsIScriptErrors, irrespective of private mode. It also shows all of the console API messages. However, it doesn't show any console API messages before open (since they don't get in the cache).
- the browser console shows all messages, like the web console.
- we had private browsing tests but they got removed (when? why?). we need tests for both the browser console and the web console in private browsing.

The above situation is not ideal.

I propose the following changes:

- I would like to add caching of window.console API messages in private browsing. I don't see why this was disabled - we remove the cache of messages every time each window is destroyed. We can't leak these messages.
- I think there's value in showing cached messages in private browsing when one wants to debug a web app. It's also a matter of consistency.
- thus I want the web console to show all incoming messages, irrespective of any private browsing flags. This is transient information that can and should be displayed.
- when opened the browser console must not display *cached* script errors/warnings nor window.console API messages that come from private windows. However, if you do keep the browser console open I believe it makes sense for it to show errors/warnings/messages that are live. This would help with debugging chrome errors.
- add tests for all of the mentioned cases.

Any thoughts?

Comment 1

[Mihai Sucan [:msucan]]

Ehsan: can you please look at comment #0 and let us know if it makes sense or if we should do things differently? Thank you!

Comment 2

[Josh Matthews [:jdm]]

I think all of your changes make sense.

Comment 3

[(no longer active)]

(In reply to Mihai Sucan [:msucan] from comment #0)
> - I would like to add caching of window.console API messages in private
> browsing. I don't see why this was disabled - we remove the cache of
> messages every time each window is destroyed. We can't leak these messages.

This is an in-memory cache, right?  If yes, then that's fine.

> - I think there's value in showing cached messages in private browsing when
> one wants to debug a web app. It's also a matter of consistency.

Agreed.

> - thus I want the web console to show all incoming messages, irrespective of
> any private browsing flags. This is transient information that can and
> should be displayed.

Hmm, web console is per-tab, right?  Then why would it make sense for it to do any filtering at all?

> - when opened the browser console must not display *cached* script
> errors/warnings nor window.console API messages that come from private
> windows. However, if you do keep the browser console open I believe it makes
> sense for it to show errors/warnings/messages that are live. This would help
> with debugging chrome errors.

The browser console is the chrome version of the web console, right?  If yes, that should be able to see all messages, irrespective of whether they come from a private window, I think.  But we need to make sure that it doesn't persist those messages to disk, and we also need to make sure that when a private window is closed, all of its messages are cleared from the cache, since we don't want somebody to be able to sit at your computer after you closed a private window, open a browser console and see what you've been doing.  This is the original reason why we had restrictions over messages coming from private windows.

> - add tests for all of the mentioned cases.

Absolutely!

Comment 4

[Mihai Sucan [:msucan]]

Thank you Josh and Ehsan!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> (In reply to Mihai Sucan [:msucan] from comment #0)
> > - I would like to add caching of window.console API messages in private
> > browsing. I don't see why this was disabled - we remove the cache of
> > messages every time each window is destroyed. We can't leak these messages.
> 
> This is an in-memory cache, right?  If yes, then that's fine.
> 
> > - I think there's value in showing cached messages in private browsing when
> > one wants to debug a web app. It's also a matter of consistency.
> 
> Agreed.
> 
> > - thus I want the web console to show all incoming messages, irrespective of
> > any private browsing flags. This is transient information that can and
> > should be displayed.
> 
> Hmm, web console is per-tab, right?  Then why would it make sense for it to
> do any filtering at all?

Correct. Web console is per-tab and what I am saying here is it should not do any filtering at all in relation to private browsing.


> > - when opened the browser console must not display *cached* script
> > errors/warnings nor window.console API messages that come from private
> > windows. However, if you do keep the browser console open I believe it makes
> > sense for it to show errors/warnings/messages that are live. This would help
> > with debugging chrome errors.
> 
> The browser console is the chrome version of the web console, right?

Yes.

> If yes, that should be able to see all messages, irrespective of whether they
> come from a private window, I think.  But we need to make sure that it
> doesn't persist those messages to disk, and we also need to make sure that
> when a private window is closed, all of its messages are cleared from the
> cache, since we don't want somebody to be able to sit at your computer after
> you closed a private window, open a browser console and see what you've been
> doing.  This is the original reason why we had restrictions over messages
> coming from private windows.

Sounds good. What I propose should cover this:

- the web console / browser console never store messages to disk.
- the web console will show all messages, even cached messages for private tabs.
- the browser console will show all messages, even from private windows, during live usage, but *not* cached messages from private windows. this means that if you open the browser console after you close a private window you will see nothing.

I've already started work on this patch.

Comment 5

[Josh Matthews [:jdm]]

So just to make it explicit, if you leave the browser console open while you close the last private window, the messages from private windows will remain until you close the browser console?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Josh Matthews [:jdm] from comment #5)
> So just to make it explicit, if you leave the browser console open while you
> close the last private window, the messages from private windows will remain
> until you close the browser console?

Yes. Would you expect messages are cleared?

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Rob: this implements the behavior we discussed in the bug and adds a test that checks things work as expected.

Asking Ehsan for feedback so he has a chance to check this is working as intended.

Please let me know if I should make any changes. Thank you both!

Comment 8

[(no longer active)]

(In reply to comment #6)
> (In reply to Josh Matthews [:jdm] from comment #5)
> > So just to make it explicit, if you leave the browser console open while you
> > close the last private window, the messages from private windows will remain
> > until you close the browser console?
> 
> Yes. Would you expect messages are cleared?

Yes, I would.  I think we need to handle this case.

The rest makes sense to me though.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> (In reply to comment #6)
> > (In reply to Josh Matthews [:jdm] from comment #5)
> > > So just to make it explicit, if you leave the browser console open while you
> > > close the last private window, the messages from private windows will remain
> > > until you close the browser console?
> > 
> > Yes. Would you expect messages are cleared?
> 
> Yes, I would.  I think we need to handle this case.

I would not expect that. If I close the private window I would find it surprising to see all the messages disappear. I see the browser console as a log of things that happened, which is not tied to any tab or window. As a developer I pick to open it to record what happens. If I close it, then I choose to loose all those private messages.

Is this something that you hold strongly for?

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> > (In reply to comment #6)
> > > (In reply to Josh Matthews [:jdm] from comment #5)
> > > > So just to make it explicit, if you leave the browser console open while you
> > > > close the last private window, the messages from private windows will remain
> > > > until you close the browser console?
> > > 
> > > Yes. Would you expect messages are cleared?
> > 
> > Yes, I would.  I think we need to handle this case.
> 
> I would not expect that. If I close the private window I would find it
> surprising to see all the messages disappear. I see the browser console as a
> log of things that happened, which is not tied to any tab or window. As a
> developer I pick to open it to record what happens. If I close it, then I
> choose to loose all those private messages.
> 
> Is this something that you hold strongly for?

Yes.  I think surprising people in this way is better than leaving data around which can be used to figure out what you have been doing.  It's not always evident whether there is a Browser Console window open when you're doing something in PB mode.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> Yes.  I think surprising people in this way is better than leaving data
> around which can be used to figure out what you have been doing.  It's not
> always evident whether there is a Browser Console window open when you're
> doing something in PB mode.

No problem. However, given the current design of things messages in the browser console have no ownership and there's no flag to mark a message as coming from a private window.

To allow the removal of messages once their window is destroyed would be overkill - we use the remote debugging protocol for all of this stuff and we would need to add the window IDs to the protocol packets. Plus we need a notification for when specific windows are destroyed.

We could also do something simpler: only add a "isPrivate" flag to all of the private message and clear them all without discrimination when a private window is closed - for this we still need a new packet to notify the console client that it needs to cleanup the output.

Would it be "too much" to add the isPrivate flag? I am inclined to say yes, there's not much value added for this special casing, but I am not sure, especially when I think of b2g, firefox devs and addon/extension/app devs. I think they would want access to messages that happen during private browsing. If we don't provide them, we make it harder to debug errors/problems that happen during private browsing - which is something we would like to avoid (errors during private browsing can lead to data leaks). Anyone has any opinions on this?

Another option is to change the patch to do what the error console does: don't show any private script errors or console API messages at all in the browser console.

Would it be acceptable to add a session-only* pref to show private messages in the browser console? It would be useful for firefox and b2g devs (including those who work on addons).


* session-only means something like the network response body logging option. This is always off by default to prevent slowing down stuff when the users enable it and forget turning it off. Same would be with "show private messages": if you close and reopen the browser console you no longer have the option enabled - you have to enable it explicitly each time.

Comment 12

[(no longer active)]

I was thinking of using last-pb-context-exited which fires when the *last* PB window is closed to clear all messages coming from any private windows.  Doing that will need the isPrivate flag you're talking about though.  Is that less crazy?

Comment 13

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> I was thinking of using last-pb-context-exited which fires when the *last*
> PB window is closed to clear all messages coming from any private windows. 
> Doing that will need the isPrivate flag you're talking about though.  Is
> that less crazy?

I did not know of the last-pb-context-exited notification. This and the isPrivate flag for message should be sufficient for what we need and definitely less crazy. Thank you!

Comment 14

[Rob Campbell [:rc] (:robcee)]

This makes sense to me, though I find it a bit heavy to impose this kind of work on a debugging tool someone opened to check something while in private browsing mode. Feels like overkill.

That said, the steps outlined sound like a good solution if this is a thing we need to do.

Does the current Error Console take any steps to clear messages from PB-windows?

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 752311 [details] [diff] [review]
proposed patch

patch looks good.

Comment 16

[(no longer active)]

(In reply to comment #14)
> This makes sense to me, though I find it a bit heavy to impose this kind of
> work on a debugging tool someone opened to check something while in private
> browsing mode. Feels like overkill.

Well, you don't want a tech-savvy person to use a debugging tool to, erm, discover engagement plans that their partner is thinking about to see which websites they've been visiting on their own, right?  :-)

> Does the current Error Console take any steps to clear messages from
> PB-windows?

Yes.

Comment 17

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> (In reply to comment #14)
> > This makes sense to me, though I find it a bit heavy to impose this kind of
> > work on a debugging tool someone opened to check something while in private
> > browsing mode. Feels like overkill.
> 
> Well, you don't want a tech-savvy person to use a debugging tool to, erm,
> discover engagement plans that their partner is thinking about to see which
> websites they've been visiting on their own, right?  :-)
> 
> > Does the current Error Console take any steps to clear messages from
> > PB-windows?
> 
> Yes.

The error console simply does not display any private messages, at all.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch: added a "private" flag for console API messages, script errors and network events, added a "lastPrivateContextExited" notification, updated the Browser Console to clear the private messages when the notification happens, and updated the test accordingly.

Please check the test or apply the patch and see if there's anything I should change. Thank you!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=aaec677fd69b

Note that I've been bitten by an interesting implementation fact of last-pb-context-exiting: if I call chromeWindow.close() the said notification does not happen because warnAboutClosingWindow() from browser.js does not execute. I had to use BrowserTryToCloseWindow(). Doesn't this open the potential of leaks for addons/extensions? last-pb-context-exiting doesn't always fire and the docs say this is what addon authors should listen for if they want to clear data...

Also last-pb-context-exiting notifies us that the close is about to happen and one can cancel the close. This means that when the user prevents the private context from exiting, the browser console will still clear the private messages. This is not really a problem blocking this patch, but it's worth mentioning that we should have a notification that tells us when last-pb-context *exited*.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: interdiff

interdiff for rob to review. Please let me know if you have any concerns. Thank you!

Comment 20

[Josh Matthews [:jdm]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> Note that I've been bitten by an interesting implementation fact of
> last-pb-context-exiting: if I call chromeWindow.close() the said
> notification does not happen because warnAboutClosingWindow() from
> browser.js does not execute. I had to use BrowserTryToCloseWindow(). Doesn't
> this open the potential of leaks for addons/extensions?
> last-pb-context-exiting doesn't always fire and the docs say this is what
> addon authors should listen for if they want to clear data...

Interesting you should note this; I, too, encountered it and filed bug 845893, which was shot down.
 
> Also last-pb-context-exiting notifies us that the close is about to happen
> and one can cancel the close. This means that when the user prevents the
> private context from exiting, the browser console will still clear the
> private messages. This is not really a problem blocking this patch, but it's
> worth mentioning that we should have a notification that tells us when
> last-pb-context *exited*.

last-pb-context-exited is a real notification that does exactly that.

Comment 21

[Mihai Sucan [:msucan]]

(In reply to Josh Matthews [:jdm] from comment #20)
> > Also last-pb-context-exiting notifies us that the close is about to happen
> > and one can cancel the close. This means that when the user prevents the
> > private context from exiting, the browser console will still clear the
> > private messages. This is not really a problem blocking this patch, but it's
> > worth mentioning that we should have a notification that tells us when
> > last-pb-context *exited*.
> 
> last-pb-context-exited is a real notification that does exactly that.

Huh, confusing. I just updated my local patch and it works, indeed. Thanks! I think my confusion showed up because I searched MDN for the notification ehsan mentioned (last-pb-context-exited) and the result I clicked was about -exiting.

I will also update the patch here after I get feedback/reviews.

Comment 22

[(no longer active)]

Comment on attachment 753440 [details] [diff] [review]
updated patch

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

Note that I did a very high-level scan through the patch.  IOW, I'm mostly f+ing the idea.

::: browser/devtools/webconsole/test/browser_console_private_browsing.js
@@ +31,5 @@
> +  ok(win, "new window");
> +  ok(PrivateBrowsingUtils.isWindowPrivate(win), "window is private");
> +
> +  let gBrowser, content, hud, expectedMessages;
> +  whenDelayedStartupFinished(win, () => {

Ewww.  What happened to the good old function?  :(

::: toolkit/devtools/server/actors/webconsole.js
@@ +1054,5 @@
> +          this._dbgGlobals.delete(windowId);
> +        }
> +        break;
> +      }
> +      case "last-pb-context-exiting":

Use last-pb-context-exited please.

Comment 23

[Rob Campbell [:rc] (:robcee)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22)
> Comment on attachment 753440 [details] [diff] [review]
> updated patch
> 
> Review of attachment 753440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note that I did a very high-level scan through the patch.  IOW, I'm mostly
> f+ing the idea.
> 
> ::: browser/devtools/webconsole/test/browser_console_private_browsing.js
> @@ +31,5 @@
> > +  ok(win, "new window");
> > +  ok(PrivateBrowsingUtils.isWindowPrivate(win), "window is private");
> > +
> > +  let gBrowser, content, hud, expectedMessages;
> > +  whenDelayedStartupFinished(win, () => {
> 
> Ewww.  What happened to the good old function?  :(

Some (most?) of the devtoolers are a tad enthusiastic when it comes to using new JS features. Fat-arrow functions do have the nice property of being bound to the containing context though.

:)

Comment 24

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 753442 [details] [diff] [review]
interdiff

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

::: toolkit/devtools/webconsole/NetworkHelper.jsm
@@ +212,5 @@
>      try {
>        return this.getRequestLoadContext(aRequest).associatedWindow;
> +    } catch (ex) {
> +      // getWindowForRequest() throws on b2g: there is no associatedWindow
> +      // property.

should we put something on the console anyway here?

Comment 25

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22)
> Comment on attachment 753440 [details] [diff] [review]
> updated patch
> 
> Review of attachment 753440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note that I did a very high-level scan through the patch.  IOW, I'm mostly
> f+ing the idea.

Thanks!

> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +1054,5 @@
> > +          this._dbgGlobals.delete(windowId);
> > +        }
> > +        break;
> > +      }
> > +      case "last-pb-context-exiting":
> 
> Use last-pb-context-exited please.

This is included in my latest patch. I just need to get the try push to be green.

Comment 26

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #24)
> Comment on attachment 753442 [details] [diff] [review]
> interdiff
> 
> Review of attachment 753442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/webconsole/NetworkHelper.jsm
> @@ +212,5 @@
> >      try {
> >        return this.getRequestLoadContext(aRequest).associatedWindow;
> > +    } catch (ex) {
> > +      // getWindowForRequest() throws on b2g: there is no associatedWindow
> > +      // property.
> 
> should we put something on the console anyway here?

We would get at least one warning (or even more) in the console for every network request, which would be too much. I will add the bug number as a TODO in the comment in the updated patch.

Comment 27

[Mihai Sucan [:msucan]]

Attachment: test fix and rebase

This patch had a green try run yesterday: https://tbpl.mozilla.org/?tree=Try&rev=da4a2cffb39d

Thanks for reviews!

Comment 28

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/a1b21de96043

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a1b21de96043