Clear Private Data does not clear JS Console

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Preferences
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dougt, Assigned: Simon Bünzli)

Tracking

(Depends on: 1 bug, {fixed1.8.1, privacy})

unspecified
Firefox 2 beta2
fixed1.8.1, privacy
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
Clear Private Data does not clear JS Console.  There sometimes is private data in the js console.
The JS console data is not saved across sessions, why should it matter?
*** Bug 313183 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 3

12 years ago
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.

Updated

12 years ago
Keywords: privacy
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.
(Reporter)

Comment 5

12 years ago
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.

Flags: blocking1.8.1?
*** Bug 338882 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

11 years ago
Created attachment 223077 [details] [diff] [review]
clear the console service's buffer when the history is purged
Attachment #223077 - Flags: review?(mconnor)
(Assignee)

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 8

11 years ago
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).
Attachment #223098 - Flags: review?(darin)

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
Created attachment 223137 [details] [diff] [review]
less hacky solution (extends nsIConsoleService_MOZILLA_1_8_BRANCH)
Attachment #223098 - Attachment is obsolete: true
Attachment #223137 - Flags: review?(darin)
Attachment #223098 - Flags: review?(darin)

Comment 11

11 years ago
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...
(Assignee)

Comment 12

11 years ago
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).
Attachment #223137 - Attachment is obsolete: true
Attachment #223285 - Flags: review?(darin)
Attachment #223137 - Flags: review?(darin)
(Assignee)

Comment 13

11 years ago
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

11 years ago
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.
Attachment #223285 - Flags: superreview?(mconnor)
Attachment #223285 - Flags: review?(darin)
Attachment #223285 - Flags: review+
(Assignee)

Comment 15

11 years ago
Created attachment 225118 [details] [diff] [review]
corresponding branch patch (w/o compiler error)
Attachment #223287 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #223077 - Flags: review?(mconnor)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2 beta2
Will review this later today.
Flags: blocking-firefox2? → blocking-firefox2+
Comment on attachment 223285 [details] [diff] [review]
trunk patch (updated to comments)

Looks good for trunk.
Attachment #223285 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Updated

11 years ago
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
Whiteboard: [checkin needed]
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Blocks: 344955
Depends on: 345327
(Assignee)

Comment 19

11 years ago
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.
Attachment #223077 - Attachment is obsolete: true
Attachment #225118 - Attachment is obsolete: true
Attachment #229998 - Flags: approval1.8.1?
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
Attachment #229998 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed (1.8 branch)]
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
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]

Comment 22

11 years ago
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

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

~B
(Assignee)

Comment 24

11 years ago
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

11 years ago
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.
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

11 years ago
(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
(Assignee)

Comment 28

11 years ago
(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.
(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.

Updated

11 years ago
Depends on: 346372

Comment 30

11 years ago
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!
*** Bug 350587 has been marked as a duplicate of this bug. ***
*** Bug 353133 has been marked as a duplicate of this bug. ***
*** Bug 359140 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 369408
You need to log in before you can comment on or make changes to this bug.