462106 - Clear the data copied to clipboard inside the private browsing mode after leaving it

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[(no longer active)]

From bug 248970 comment 461:

We may need to clear the contents of the clipboard after leaving the private browsing mode if the data comes from inside the private browsing mode.

This bug is filed to discuss this.

Comment 1

[Mike Connor [:mconnor]]

We should clear the clipboard only if the contents came from Firefox inside of private browsing.  I don't know the clipboard implementation, but I would hope this is reasonably easy to track and verify...

Comment 2

[(no longer active)]

I agree.  Changing the summary accordingly.  I'll try to look into this after landing the PB patch and its UI, which might be after Beta 2...

Comment 3

[Fritz]

What is considered data from within Firefox?  Copy and paste of text / image from the page?  Using nightly tester tool to copy a build ID to the clipboard?  Taking a screenshot of a window?

I am also not convinced that this is any different than saving a page when you're in PB mode.  If you do that then it's quite intentional and clearly shouldn't be deleted either.

Comment 4

[(no longer active)]

I don't think it's a good comparison.  Firstly, clipboard data is not visible unless you paste it somewhere, so it's easy to forget that you've copied something you'd rather other people not see.  Secondly, I think a larger percentage of users actually know that data left on the clipboard can be misused in that way, but many know that once they save a file, there's a chance for other people to see it.  Last, but not least, there are methods to protect the files you save (you can save it on a flash memory, in a password protected storage, encrypt it, etc.) but there's no way to protect clipboard data.

Comment 5

[Igor Velkov]

There is a very, very simple way to protect clipboard: to put something new into clipboard.
It's very easy to think about man, enter into private mode to go to "strange place", get a snippet of text, switch into normal mode, and paste it into blog.

As compromise, there have to be a non-modal warning about non-empty clipboard exiting private mode, and chooser: 
clear clipboard(default), keep clipboard, stay in private mode, and checkbox "remember decision" for exit buttons.

Comment 6

[u88484]

Nominating for 3.1 since this in a way defeats the pb mode feature in a few ways.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Low bar:
 - check to see if the user copied something while in PB Mode
 - if so, empty clipboard

High bar:
 - keep record of what we copied into clipboard while in PB Mode
 - check to see if that is still in the clipboard
 - if so, empty clipboard

Comment 8

[Justin Dolske [:Dolske]]

It might be possible to keep the "was this copied from Firefox in private browsing mode" state in the clipboard itself. Clipboards generally support multiple data flavors, some state could be kept with that.

Comment 9

[u88484]

Dolske: I believe you reset the blocking-firefox3.1+ flag by accident.

Comment 10

[(no longer active)]

(In reply to comment #8)
> It might be possible to keep the "was this copied from Firefox in private
> browsing mode" state in the clipboard itself. Clipboards generally support
> multiple data flavors, some state could be kept with that.

I agree.  This way, we can tell for sure whether or not a certain piece of data is coming from private browsing mode.  Using text comparisons in prone to errors, and we may end up wiping out data copied from other applications in edge cases.

Comment 11

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 12

[(no longer active)]

Attachment: Patch (v1)

The test does not still work (see http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/677c3886c71e01ea), but I thought I could submit the patch for a preliminary review.

Comment 13

[(no longer active)]

By the way, this implements the high bar as indicated in comment 7.  The patch itself works correctly when I test it from within Firefox itself.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's call nsClipboardPrivateBrowsingHandler nsClipboardPrivacyHandler instead, to make it a bit more generic, and document in the .h file what it's for and when PrepareDataForClipboard should be called.

+  NS_ASSERTION ( aTransferable, "clipboard given a null transferable" );

Unnecessary spaces.

But instead of calling PrepareDataForClipboard in lots of places, can't we put it in one place, like in nsTransferable's constructor or something?

Comment 15

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #14)
> But instead of calling PrepareDataForClipboard in lots of places, can't we put
> it in one place, like in nsTransferable's constructor or something?

Ideally we could, but I don't think such a place exists (well, short of nsBaseClipboard which is used by a few of the platform clipboard implementations).  nsTransferable's constructor wouldn't be appropriate, because a) it's used in other places besides the clipboard code (such as drag-drop) and b) we need to do this in nsIClipboard::SetData right before setting the data, and doing this in nsTransferable's constructor would be wasteful for all those transferables which are not supposed to set clipboard data (and it would possibly break some callers because nsTransferables would suddenly start getting an additional flavor in some cases).

I also went ahead and re-wrote the test as a chrome test.  I filed a bug on the xpcshell problem (bug 475088), and I think we're safe to test is using a chrome test for now.  So, the patch is complete and ready for the real review.  :-)

Comment 16

[(no longer active)]

Landed on m-c: <http://hg.mozilla.org/mozilla-central/rev/97907892496f>

I'm going to land it on branch after a green cycle.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 358483 [details] [diff] [review]
Patch (v2)

>diff --git a/widget/src/gtk2/nsClipboard.h b/widget/src/gtk2/nsClipboard.h

>+    nsClipboardPrivacyHandler mPrivacyHandler;

>diff --git a/widget/src/xpwidgets/nsClipboardPrivacyHandler.cpp b/widget/src/xpwidgets/nsClipboardPrivacyHandler.cpp

>+NS_IMPL_ISUPPORTS2(nsClipboardPrivacyHandler, nsIObserver, nsISupportsWeakReference)

>+nsClipboardPrivacyHandler::nsClipboardPrivacyHandler()
>+{
>+  NS_ADDREF(this);

1) This will definitely confuse leak logging (see orange tinderboxes).
2) Just because you ask the observer service to take a weak reference doesn't mean that nothing will ever take a strong reference to the nsClipboardPrivacyHandler. If something does and it keeps the nsClipboardPrivacyHandler alive longer than the nsClipBoard that something will be accessing freed memory.

I'm going to back this out.

Comment 18

[Peter Van der Beken [:peterv]]

Backed out.

Comment 19

[(no longer active)]

Attachment: Patch (v3)

New approach using XPCOM-based nsClipboardPrivacyHandler instantiation.

Comment 20

[(no longer active)]

Attachment: Patch (v4)

Sorry for requesting another round of reviews, but I found out that the previous patch doesn't compile in debug mode because nsCOMPtr cannot handle multiple inheritance of nsISupports.  This patch is mostly the same, but it just factors out PrepareDataForClipboard into an interface nsIClipboardPrivacyHandler which the callers will use.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be slightly simpler to go with the original patch but use nsRefPtr instead of nsCOMPtr.

Comment 22

[(no longer active)]

Attachment: Patch (v3 using nsRefPtr)

I also added a timeout to the test.  Without that timeout, the test seemed to fail sporadically.

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9c777aa29014

Comment 24

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f22a8c4c47fa

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre

There is something suspicious when using Vmware. When I tried to verify this fix I've seen that the clipboard content is still available on the host system. I've no idea if this is a Vmware bug or not. Probably it doesn't recognize the automatically clearing of the clipboard data?

Comment 26

[(no longer active)]

(In reply to comment #25)
> There is something suspicious when using Vmware. When I tried to verify this
> fix I've seen that the clipboard content is still available on the host system.
> I've no idea if this is a Vmware bug or not. Probably it doesn't recognize the
> automatically clearing of the clipboard data?

Here is how I implemented this feature.  Inside private browsing mode, anything copied to the clipboard is accompanied by a dummy mime type (application/x-moz-private-browsing), and upon exiting, I check to see if the contents of this clipboard have a application/x-moz-private-browsing flavor, and if they do (which means it had been copied during the private mode), I write an empty data packet to the clipboard which effectively makes it empty.

If VMware doesn't handle it correctly, it's either because they don't honor the application/x-moz-private-browsing mime type (which is unknown to them) or they don't write empty data into the clipboard.  Either way, I don't think that this should be something we handle in Mozilla code.

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Add |ok(true, ...);| [Checkin: Comment 29 & 30]

Comment 28

[(no longer active)]

(In reply to comment #27)
> Created an attachment (id=369623) [details]
> (Bv1) Add |ok(true, ...);|

This needs an sr as well, right?

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 369623 [details] [diff] [review]
(Bv1) Add |ok(true, ...);|
[Checkin: Comment 29 & 30]


http://hg.mozilla.org/mozilla-central/rev/d35773ad9fe2

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 369623 [details] [diff] [review]
(Bv1) Add |ok(true, ...);|
[Checkin: Comment 29 & 30]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b8961fd2fcf4

Comment 31

[Serge Gautherie (:sgautherie)]

verified1.9.1, this part too:

Before:
{
*** 5879 INFO Running chrome://mochikit/content/chrome/widget/test/test_bug462106.xul...
*** 5881 INFO Running chrome://mochikit/content/chrome/widget/test/test_keycodes.xul...
}

After:
{
*** 5879 INFO Running chrome://mochikit/content/chrome/widget/test/test_bug462106.xul...
*** 5880 INFO TEST-PASS | chrome://mochikit/content/chrome/widget/test/test_bug462106.xul | no Private Browsing service
*** 5882 INFO Running chrome://mochikit/content/chrome/widget/test/test_keycodes.xul...
}

Comment 32

[simon.d.wyatt]

Feedback from a very annoyed user:
This "feature" is infuriating. No other application silently destroys my clipboard, so I frequently forget that Firefox does it, and end up losing something I wanted.
I agree with Fritz's comparison above to saving a file from private browsing mode, and I take offense to Ehsan Akhgari's attempted justification that the user is a moron and has to be protected from himself.
Please let me turn this misfeature off via about:config. If I accidentally leave something on my clipboard I wanted to keep private, that's my own fault and my own responsibility.

Comment 33

[yakmandango]

For a request to add a "disable" feature, please see https://bugzilla.mozilla.org/show_bug.cgi?id=815952

Comment 34

[costinel]

Mozilla, thank you for transforming into the new Microsoft "this features works as intended"
#462106
#816998
#827260
#849799
#410669