322169 - Clear Private Data does not clear JS Console

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Doug Turner (:dougt)]

Clear Private Data does not clear JS Console.  There sometimes is private data in the js console.

Comment 1

[Benjamin Smedberg]

The JS console data is not saved across sessions, why should it matter?

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 313183 has been marked as a duplicate of this bug. ***

Comment 3

[Doug Turner (:dougt)]

use case.  names have been changed to protect the guilty.

1) user goes to some seriously nasty sites.
2) wife comes home.
3) user goes to Tools / Clear Private Data...
4) user selects all checkboxes, and hits enter.
5) wife asks to login to webmail to check her mail.
6) webmail site doesn't layout properly
7) wife debugs webpage using js console.
8) user in shitload of trouble.

seriously -- if you are suggesting that the user exits out of the application (ie, out of the session) before "Clear Private Data..." is completed successfully, then we should let the user know this via a dialog.

Comment 4

[Majken Connor [:Kensie]]

ok, if you're going to try and sneak around someone's back, don't marry someone who smart enough to catch you.

b) a better example would be someone buying an engagement ring for their wife, but doesn't want her to know that he's been shopping around.

Comment 5

[Doug Turner (:dougt)]

abstracting the use case we get to the same thing (i was _trying_ to be funny).  Currently if you do not exit the browser your "Private Data" continues to exist.  There might be other places, but this is the first thing I noticed.

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 338882 has been marked as a duplicate of this bug. ***

Comment 7

[Simon Bünzli]

Attachment: clear the console service's buffer when the history is purged

Comment 8

[Simon Bünzli]

Attachment: less hacky solution (adds a reset method to nsIConsoleService)

This solution is more complete by actually allowing the console service to reset itself.

The remaining issue would be whether (and if so how) to notify the console service listeners about the reset. The proposed solution is to not notify them and add a null message before the reset in the relevant places (which also nicely ensures compatibility with extensions relying on the null messages).

Comment 9

[Darin Fisher]

For Firefox 2 (aka the Mozilla 1.8 branch), we will need to invent a new interface nsIConsoleService_MOZILLA_1_8_BRANCH that extends from nsIConsoleService on which we can define the reset method.  We have a policy in place of not changing interfaces between FF 1.5 and 2.

Comment 10

[Simon Bünzli]

Attachment: less hacky solution (extends nsIConsoleService_MOZILLA_1_8_BRANCH)

Comment 11

[Darin Fisher]

Comment on attachment 223137 [details] [diff] [review]
less hacky solution (extends nsIConsoleService_MOZILLA_1_8_BRANCH)

>Index: xpcom/base/nsIConsoleService.idl

>+/**
>+ * Temporary interface for Gecko 1.8.1. Should the console service not be
>+ * improved/replaced for Gecko 1.9 this is be merged with the above.
>+ *
>+ * @status TEMPORARY
>+ */
>+[scriptable, uuid(883472a0-ea9b-11da-8ad9-0800200c9a66)]
>+interface nsIConsoleService_MOZILLA_1_8_BRANCH : nsIConsoleService
>+{
>+    /**
>+     * Clear the message buffer (e.g. for privacy reasons).
>+     */
>+    void reset();
> };

Thanks for creating this interface.  Please go ahead and prepare a
second patch for the trunk that modifies nsIConsoleService directly.
We don't want to land any MOZILLA_1_8_BRANCH interfaces on the trunk :)


>Index: xpcom/base/nsConsoleService.cpp

>+NS_IMETHODIMP
>+nsConsoleService::Reset()
>+{
>+    /*
>+     * Make sure nobody trips into the buffer while it's being reset
>+     */
>+    nsAutoLock lock(mLock);
>+
>+    mCurrent = 0;
>+    mFull = PR_FALSE;
>+
>+    /*
>+     * Free all messages stored so far and re-initialize the buffer
>+     */
>+    for (PRUint32 i = 0; i < mBufferSize && mMessages[i] != nsnull; i++)
>+        NS_RELEASE(mMessages[i]);
>+
>+    memset(mMessages, 0, mBufferSize * sizeof(nsIConsoleMessage *));
>+
>+    return NS_OK;
>+}

A couple comments:

1. Maybe you could just loop from 0 to mCurrent instead?
2. NS_RELEASE nulls out its parameter, so the memset is redundant.


>+            this.mCService = isupports.QueryInterface(Components.interfaces.nsIConsoleService_MOZILLA_1_8_BRANCH);

Since the component implements nsIClassInfo, you should be able to remove
the explicit QueryInterface altogether :-)


I had to stop reviewing here.  Will continue later...

Comment 12

[Simon Bünzli]

Attachment: trunk patch (updated to comments)

(In reply to comment #11)
> 1. Maybe you could just loop from 0 to mCurrent instead?
No: when mFull is NS_TRUE the buffer has wrapped around and we'll have to make sure to release all 250 slots (cf. destructor).

Comment 13

[Simon Bünzli]

Attachment: corresponding branch patch

(In reply to comment #11)
> Since the component implements nsIClassInfo, you should be able to remove
> the explicit QueryInterface altogether :-)
I prefer to leave that to a clean-up bug.

Comment 14

[Darin Fisher]

Comment on attachment 223285 [details] [diff] [review]
trunk patch (updated to comments)

>Index: toolkit/components/console/content/consoleBindings.xml

>+              if (msg.message)
>+                this.appendMessage(msg.message);
>+              else // observed a null/"clear" message
>+                this.clearConsole();

It is generally good for readability to use {}'s whenever coding
an else statement.  I recommend the following:

               if (msg.message) {
                 this.appendMessage(msg.message);
               } else { // observed a null/"clear" message
                 this.clearConsole();
               }


OK, r=darin

I'd like mconnor to also review this change.

Comment 15

[Simon Bünzli]

Attachment: corresponding branch patch (w/o compiler error)

Comment 16

[Mike Connor [:mconnor]]

Will review this later today.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 223285 [details] [diff] [review]
trunk patch (updated to comments)

Looks good for trunk.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/nsBrowserGlue.js 	1.20
mozilla/toolkit/components/console/content/consoleBindings.xml 	1.12
mozilla/xpcom/base/nsConsoleService.cpp 	3.22
mozilla/xpcom/base/nsIConsoleService.idl 	3.6

Comment 19

[Simon Bünzli]

Attachment: branch patch (including fix for bug 345327)

Drivers: This patch extends the console service with a Reset method (on a branch-specific interface), makes the console use it and adds an observer for Clear Private Data which also triggers it. Hardly risky.

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 229998 [details] [diff] [review]
branch patch (including fix for bug 345327)

a=mconnor on behalf of drivers for checkin to the 1.8 branch

Comment 21

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/console/content/consoleBindings.xml 	1.9.2.3
mozilla/browser/components/nsBrowserGlue.js 	1.4.2.12
mozilla/xpcom/base/nsConsoleService.h 	3.7.8.1
mozilla/xpcom/base/nsConsoleService.cpp 	3.19.8.1
mozilla/xpcom/base/nsIConsoleService.idl 	3.5.28.1

Comment 22

[Bryan]

Not sure if this is the cause but using build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060725 BonEcho/2.0b1 ID:2006072518 I no longer have any data in the Error Console but did in the previous build (build prior to this checkin).  This occurs in both FF modes (Normal/Safe Mode).  The Error Console is "empty" for every single site I go to in both modes.

~B

Comment 23

[Bryan]

Also... Shouldn't there be an option to enable/disable this in the configuration for "Clear Private Data" to be consistent?

~B

Comment 24

[Simon Bünzli]

Attachment: fix for checkin issue (1.8 branch)

Gavin: It seems that this one line was somehow garbled during check-in. Could you please correct it?

Comment 25

[Adam Guthrie]

Bryan, please feel free to file a follow-up bug on the issue you mentioned and make it depend/block this bug, but commenting in this bug is not very productive.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Gah, sorry. To get your patch to apply I needed to replace the instance of MOZILLA_1_8 in the file paths with mozilla, but I wasn't careful enough to verify that the changes I made didn't affect other parts of the patch.

I checked in the fix.

Comment 27

[Bryan]

(In reply to comment #26)
> Gah, sorry. To get your patch to apply I needed to replace the instance of
> MOZILLA_1_8 in the file paths with mozilla, but I wasn't careful enough to
> verify that the changes I made didn't affect other parts of the patch.
> 
> I checked in the fix.

Well after the fix for the fix "Clear Private Data" now does not clear the Error Console in both FF modes (Normal / Safe Mode) using build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060725 BonEcho/2.0b1 ID:2006072601

~B

Comment 28

[Simon Bünzli]

(In reply to comment #27)
> Well after the fix for the fix "Clear Private Data" now does not clear the
> Error Console

Indeed. If LXR now has finally caught up with Gavin's check-in, then it seems that the s/mozilla/MOZILLA_1_8/g issue affects the whole patch (i.e. also nsBrowserGlue.js, nsIConsoleService.idl and nsConsoleService.*).

Not sure what the best solution is for this issue. Maybe backing both branch changes out and reapplying the original branch patch with s|MOZILLA_1_8/|mozilla/|g ?

Anyway, sorry for these issues. I'll look into making branch patches which apply immediately.

BTW: Gavin, maybe you could check-in the patch to bug 345327 as well. It's got implicit r+ through the a+ to the branch patch and is probably annoying the heck out of some trunk testers.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> Indeed. If LXR now has finally caught up with Gavin's check-in, then it seems
> that the s/mozilla/MOZILLA_1_8/g issue affects the whole patch (i.e. also
> nsBrowserGlue.js, nsIConsoleService.idl and nsConsoleService.*).

Argh! I had already fixed the compiled files, since the change had caused bustage, but I didn't think to check the other files. I've fixed the one occurrence that LXR finds (in nsBrowserGlue), and tested that it does indeed fix the Clear button issue.

Comment 30

[Bryan]

I just filed BUG: https://bugzilla.mozilla.org/show_bug.cgi?id=346372 requesting a Clear private data option enhancement.  Now that this bug has landed
and clears the Error Console every time I "Clear Private Data" I have no way of
configuring this in the "Clear Private Data - Select items to be cleared
dialog".  Many users would expect this to be configurable by way of a seperate
"Error Console" checkbox!

Comment 31

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 350587 has been marked as a duplicate of this bug. ***

Comment 32

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 353133 has been marked as a duplicate of this bug. ***

Comment 33

[:Gavin Sharp [email: [e-mail]]]

*** Bug 359140 has been marked as a duplicate of this bug. ***