Last Comment Bug 322169 - Clear Private Data does not clear JS Console
: Clear Private Data does not clear JS Console
Status: RESOLVED FIXED
: fixed1.8.1, privacy
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 2 beta2
Assigned To: Simon Bünzli
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
: 313183 338882 350587 353133 359140 369408 (view as bug list)
Depends on: 346372 345327
Blocks: 344955
  Show dependency treegraph
 
Reported: 2006-01-02 18:25 PST by Doug Turner (:dougt)
Modified: 2007-02-05 15:12 PST (History)
17 users (show)
mconnor: blocking‑firefox2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
clear the console service's buffer when the history is purged (5.51 KB, patch)
2006-05-23 12:24 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
less hacky solution (adds a reset method to nsIConsoleService) (10.08 KB, patch)
2006-05-23 14:27 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
less hacky solution (extends nsIConsoleService_MOZILLA_1_8_BRANCH) (12.64 KB, patch)
2006-05-23 23:04 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
trunk patch (updated to comments) (9.99 KB, patch)
2006-05-25 03:31 PDT, Simon Bünzli
darin.moz: review+
mconnor: superreview+
Details | Diff | Splinter Review
corresponding branch patch (12.61 KB, patch)
2006-05-25 03:33 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
corresponding branch patch (w/o compiler error) (12.83 KB, patch)
2006-06-10 05:04 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
branch patch (including fix for bug 345327) (13.33 KB, patch)
2006-07-20 13:04 PDT, Simon Bünzli
mconnor: approval1.8.1+
Details | Diff | Splinter Review
fix for checkin issue (1.8 branch) (1.51 KB, patch)
2006-07-26 00:05 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review

Description User image Doug Turner (:dougt) 2006-01-02 18:25:41 PST
Clear Private Data does not clear JS Console.  There sometimes is private data in the js console.
Comment 1 User image Benjamin Smedberg [:bsmedberg] 2006-01-02 18:38:26 PST
The JS console data is not saved across sessions, why should it matter?
Comment 2 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2006-01-02 18:45:21 PST
*** Bug 313183 has been marked as a duplicate of this bug. ***
Comment 3 User image Doug Turner (:dougt) 2006-01-02 18:47:54 PST
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 User image Majken Connor [:Kensie] 2006-01-02 18:53:13 PST
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 User image Doug Turner (:dougt) 2006-01-02 19:07:28 PST
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 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2006-05-22 13:56:52 PDT
*** Bug 338882 has been marked as a duplicate of this bug. ***
Comment 7 User image Simon Bünzli 2006-05-23 12:24:33 PDT
Created attachment 223077 [details] [diff] [review]
clear the console service's buffer when the history is purged
Comment 8 User image Simon Bünzli 2006-05-23 14:27:32 PDT
Created attachment 223098 [details] [diff] [review]
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 User image Darin Fisher 2006-05-23 20:01:16 PDT
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 User image Simon Bünzli 2006-05-23 23:04:10 PDT
Created attachment 223137 [details] [diff] [review]
less hacky solution (extends nsIConsoleService_MOZILLA_1_8_BRANCH)
Comment 11 User image Darin Fisher 2006-05-24 19:29:28 PDT
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 User image Simon Bünzli 2006-05-25 03:31:36 PDT
Created attachment 223285 [details] [diff] [review]
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 User image Simon Bünzli 2006-05-25 03:33:43 PDT
Created attachment 223287 [details] [diff] [review]
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 User image Darin Fisher 2006-06-01 20:41:54 PDT
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 User image Simon Bünzli 2006-06-10 05:04:26 PDT
Created attachment 225118 [details] [diff] [review]
corresponding branch patch (w/o compiler error)
Comment 16 User image Mike Connor [:mconnor] 2006-07-13 12:26:33 PDT
Will review this later today.
Comment 17 User image Mike Connor [:mconnor] 2006-07-17 09:50:55 PDT
Comment on attachment 223285 [details] [diff] [review]
trunk patch (updated to comments)

Looks good for trunk.
Comment 18 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-19 05:52:41 PDT
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 User image Simon Bünzli 2006-07-20 13:04:52 PDT
Created attachment 229998 [details] [diff] [review]
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 User image Mike Connor [:mconnor] 2006-07-21 11:06:15 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-25 14:06:03 PDT
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 User image Bryan 2006-07-25 21:03:20 PDT
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 User image Bryan 2006-07-25 21:35:46 PDT
Also... Shouldn't there be an option to enable/disable this in the configuration for "Clear Private Data" to be consistent?

~B
Comment 24 User image Simon Bünzli 2006-07-26 00:05:14 PDT
Created attachment 230706 [details] [diff] [review]
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 User image Adam Guthrie 2006-07-26 00:17:42 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-26 00:17:50 PDT
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 User image Bryan 2006-07-26 05:27:54 PDT
(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 User image Simon Bünzli 2006-07-26 06:48:26 PDT
(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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-26 11:45:45 PDT
(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 User image Bryan 2006-07-28 18:25:03 PDT
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 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2006-08-29 07:16:10 PDT
*** Bug 350587 has been marked as a duplicate of this bug. ***
Comment 32 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2006-09-18 02:43:18 PDT
*** Bug 353133 has been marked as a duplicate of this bug. ***
Comment 33 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-01 23:34:55 PST
*** Bug 359140 has been marked as a duplicate of this bug. ***
Comment 34 User image Matthias Versen [:Matti] 2007-02-05 15:12:51 PST
*** Bug 369408 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.