Closed Bug 722872 Opened 12 years ago Closed 12 years ago

nsClipboardPrivacyHandler uses global Private Browsing state to manipulate clipboard data

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: jdm, Assigned: ehsan.akhgari)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(3 files, 1 obsolete file)

Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper methods
71.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2: Move the nsClipboardPrivacyHandler implementation aways from using the global private browsing service
6.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
Assertion stack
11.02 KB, text/plain
Details 
The global PB service is going away. We need to figure out what to do with clipboard data when PB-per-window becomes a reality.
After taking a closer look at nsClipboardPrivacyHandler, here's my proposed action plan:
* add an nsILoadContext member to nsTransferable, and Init(nsILoadContext*) and GetPrivacyContext methods to nsITransferable.
* assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
* use this privacy context in nsClipboardPrivacyHandler in place of the global service - default to non-private if the context is null

Robert, are you a good person to approve this plan? Does it sound ok to you?
Ehsan is probably as good or better to approve this, but it sounds good to me.
Comment 1 sounds great to me.  I'll try to knock up a patch!
Don't try too hard, Ehsan; Diana's been looking at this, so there's no hurry.
Please note that in at least one case <http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#4815>, we don't have access to a docshell/window when we create the transferable object.  So, we may also need to add an nsILoadContext property to the nsITransferable interface to make it possible for callers to set the load context when one becomes available.

Note that in this particular case, I'm not sure if that's needed.  The scenario we care about is when somebody copies something inside of the PB mode.  So, I'd totally take a patch which just passes null to init() there and adds a good comment on why that is OK.  But the first paragraph might be necessary in other places.
Assignee: nobody → aem_koenraadt
Ehsan: By 'this particular case', do you mean this particular bug? As in, the bug focuses on copying, in which case there is always a context available? If so, shouldn't a second bug report be created that depends on this one?
(In reply to Diana Koenraadt from comment #6)
> Ehsan: By 'this particular case', do you mean this particular bug? As in,
> the bug focuses on copying, in which case there is always a context
> available? If so, shouldn't a second bug report be created that depends on
> this one?

I meant the code that I linked to in comment 5.  The goal of nsClipboardPrivacyHandler is to tag the things we copy to the clipboard inside PB mode, and to clear the contents of the clipboard when closing the last PB window if they've been tagged.  So, you only need to worry about setting the correct load context on transferables that are created when copying something to the clipboard, but for consistency it's a good idea to do our best to set the correct load context for all transferables.  It just wouldn't matter for the specific case of the code in comment 5.
This will be slightly more complicated than I originally envisioned. As Diana discovered, we can't simply store the nsILoadContext value in the transferable - this causes the last-pb-context-destroyed notification to be delayed until the clipboard is cleared, since the transferable on the clipboard is keeping the last private docshell alive. I think we need to change the |nsILoadContext privacyContext| attribute to |boolean privacyStatusOfOwner|, which corresponds to a boolean member of nsClipboardPrivacyHandler. nsCPH can implement the nsIPrivacyTransitionObserver interface and update this boolean based on the values observed in the transition handler that's called. See bug 722942 for an example of how this interface is used (and how to make sure that nsCPH properly supports nsISupportsWeakReference, which is required for this transition observer interface).
That makes sense to me.
Attached file Work in progress (obsolete) — 
I'm attaching a diff of my work in progress and am unassigning myself from this bug. Other things are taking up all my time at the moment, unfortunately.

Quote jdm:
"So, I just reread my last comment in the bug, and I am no longer sure
why it needs to be as complicated as I described. It would probably be
unintuitive if the clipboard was cleared when you closed the private
tab that was the source of the transferable item. Instead, I think we
can do this really easily:
* observe the last-pb-context-exited notification, and clear the
clipboard if the transferable is from a private context
* add a boolean attribute to nsITransferable that returns its privacy status
* add a method setOriginPrivacyStatus that takes an nsILoadContext,
which causes the boolean value to update

We can leave the Init work, I expect. It is probably just a matter of
find/replace if a reviewer disagrees."
What still needs to be done is to supply an nsILoadContext to the Init method, or setting the privacy status if a loadcontext becomes available at a later time.
Assignee: diana → nobody
Hmm, so I now think that we should store a boolean in the transferable object which represents the privacy state of the creating docshell.  But I believe that it would be better for the Init() method to still take a loading context parameter, just to make sure that people who create new transferables understand what this parameter is about, and avoid passing made-up boolean values.  (Note that passing null should still be possible where we don't know the loading context, or where it doesn't matter).

Then the rest of Josh's suggestion in comment 10 can be implemented on top of that.

Josh, do you agree whether it's sensible?
Yep, sounds good to me.
This patch does the following:

* It adds nsITransferable::Init(nsILoadContext*).  The load context might be null, which means that the transferable is non-private, but if it's non-null, we extract the boolean value for the privacy mode and store it in the transferable.
* It adds checks in debug builds to make sure that Init is always called, in form of fatal assertions.
* It adds nsIDOMDocument* agruments to nsIClipboardHelper methods which represent the document that the string is coming from.  nsIClipboardHelper implementation internally gets the nsILoadContext from that and passes it on to the transferable upon creation.  The reason that I did this was that nsIClipboardHelper is supposed to be a high-level helper, and in most of its call sites, we have easy access to a document object.
* It modifies all of the call sites of the above interfaces according to this change.

I'd like to land this as soon as possible since it has a high potential for bitrot, so a quick review is much appreciated (even though the patch is sort of large.)  Thanks!  :-)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #636714 - Flags: review?(roc)
Keywords: dev-doc-needed
Attachment #632691 - Attachment is obsolete: true
Using the infrastructure in part 1, this patch actually accomplishes what this bug is about.
Attachment #636724 - Flags: review?(roc)
Comment on attachment 636714 [details] [diff] [review]
Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper methods

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

::: editor/libeditor/text/nsPlaintextDataTransfer.cpp
@@ +51,5 @@
>    // Get the nsITransferable interface for getting the data from the clipboard
>    if (transferable) {
> +    nsCOMPtr<nsIDocument> destdoc = GetDocument();
> +    nsCOMPtr<nsISupports> container = destdoc ? destdoc->GetContainer() : nsnull;
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container);

Seems to be worth adding a helper nsIDocument::GetLoadContext() returning a (non-addrefed) nsILoadContext* for the document's container, or null. You can use that in lots of places in this patch.

And maybe add nsEditor::GetLoadContext() as well.
Attachment #636714 - Flags: review?(roc) → review+
(I didn't end up doing the nsEditor::GetLoadContext() method, since there's only three potential call sites for it, and I don't think it makes a lot of sense on nsEditor.)
https://hg.mozilla.org/mozilla-central/rev/93e55dcf0e2e
https://hg.mozilla.org/mozilla-central/rev/dbf7b4c5e8fc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
> -  void copyStringToClipboard(in AString aString, in long aClipboardID);
> +  void copyStringToClipboard(in AString aString, [optional] in nsIDOMDocument aDoc, in long aClipboardID);

Won't it be better to put the |[optional] in nsIDOMDocument aDoc| at the end to make this method backward compatible for extensions that need to work in more than one version of Firefox/Thunderbird/etc?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> I'm hitting the assert at
> http://hg.mozilla.org/mozilla-central/annotate/081d8578beb1/widget/xpwidgets/
> nsTransferable.cpp#l447 on my local Windows builds.

A backtrace would be swell, if it's available.
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 769588
Blocks: 769603
(In reply to Philip Chee from comment #21)
> > -  void copyStringToClipboard(in AString aString, in long aClipboardID);
> > +  void copyStringToClipboard(in AString aString, [optional] in nsIDOMDocument aDoc, in long aClipboardID);
> 
> Won't it be better to put the |[optional] in nsIDOMDocument aDoc| at the end
> to make this method backward compatible for extensions that need to work in
> more than one version of Firefox/Thunderbird/etc?

Are you even allowed a non-trailing optional parameter?
Comment on attachment 636714 [details] [diff] [review]
Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper methods

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

Sorry to jump in, just a quick note...

::: browser/devtools/webconsole/HUDService.jsm
@@ +997,4 @@
>          strings.push("[" + timestampString + "] " + item.clipboardText);
>        }
>      }
> +    clipboardHelper.copyString(strings.join("\n"), this.doc);

Here |this.doc| is undefined.
Depends on: 769653
(In reply to Philip Chee from comment #21)
> > -  void copyStringToClipboard(in AString aString, in long aClipboardID);
> > +  void copyStringToClipboard(in AString aString, [optional] in nsIDOMDocument aDoc, in long aClipboardID);
> 
> Won't it be better to put the |[optional] in nsIDOMDocument aDoc| at the end
> to make this method backward compatible for extensions that need to work in
> more than one version of Firefox/Thunderbird/etc?

Please file a bug on that and assign it to me.  Thanks!
Depends on: 769679
Keywords: addon-compat
Depends on: 769881
Blocks: 769882
Comment on attachment 636714 [details] [diff] [review]
Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper methods

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>@@ -1117,7 +1117,7 @@ nsContextMenu.prototype = {
> 
>     var clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
>                     getService(Ci.nsIClipboardHelper);
>-    clipboard.copyString(addresses);
>+    clipboard.copyString(addresses, document);
So, when you say per-window private browsing, you mean per chrome window? (Otherwise I would have expected this to use a content document.)
Depends on: 769961
(In reply to Josh Matthews from comment #1)
> * add an nsILoadContext member to nsTransferable, and Init(nsILoadContext*)
Although this turns out to be really annoying to do from chrome, as can be seen by the need for helper functions everywhere. Slightly more useful would have been an nsISupports* parameter, and then use do_GetInterface on that to retrieve the load context.

> * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never crash, even if it's only in debug builds. If you don't think non-fatal assertions would get noticed, why not throw an exception?
(In reply to neil@parkwaycc.co.uk from comment #27)
> Comment on attachment 636714 [details] [diff] [review]
> Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called
> in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper
> methods
> 
> >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> >--- a/browser/base/content/nsContextMenu.js
> >+++ b/browser/base/content/nsContextMenu.js
> >@@ -1117,7 +1117,7 @@ nsContextMenu.prototype = {
> > 
> >     var clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> >                     getService(Ci.nsIClipboardHelper);
> >-    clipboard.copyString(addresses);
> >+    clipboard.copyString(addresses, document);
> So, when you say per-window private browsing, you mean per chrome window?
> (Otherwise I would have expected this to use a content document.)

The platform level feature supports this flag per docshell.

(In reply to neil@parkwaycc.co.uk from comment #28)
> (In reply to Josh Matthews from comment #1)
> > * add an nsILoadContext member to nsTransferable, and Init(nsILoadContext*)
> Although this turns out to be really annoying to do from chrome, as can be
> seen by the need for helper functions everywhere. Slightly more useful would
> have been an nsISupports* parameter, and then use do_GetInterface on that to
> retrieve the load context.

It's not that annoying, see nsIDocument::GetLoadContext.

> > * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
> Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never
> crash, even if it's only in debug builds. If you don't think non-fatal
> assertions would get noticed, why not throw an exception?

Transferable objects which have not been initialized are not a valid state of the program any more.  Unfortunately XPCOM doesn't provide us with a better way of ensuring this.
OS: All → Mac OS X
Hardware: All → x86
Depends on: 769440
(In reply to neil@parkwaycc.co.uk from comment #28)
> > * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
> Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never
> crash, even if it's only in debug builds. If you don't think non-fatal
> assertions would get noticed, why not throw an exception?

+1
I was surprised to see jetpack unit test crashing because of this. I think that it is the first time I see some crashing assertions that have to be fixed in JS side. The issue in using such assertion is that it will only fail in debug builds. So that no addons developers will see that something is wrong about this. Throwing an exception or printing a warning/error message would be better options.
(In reply to Alexandre Poirot (:ochameau) from comment #30)
> (In reply to neil@parkwaycc.co.uk from comment #28)
> > > * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
> > Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never
> > crash, even if it's only in debug builds. If you don't think non-fatal
> > assertions would get noticed, why not throw an exception?
> 
> +1
> I was surprised to see jetpack unit test crashing because of this. I think
> that it is the first time I see some crashing assertions that have to be
> fixed in JS side. The issue in using such assertion is that it will only
> fail in debug builds. So that no addons developers will see that something
> is wrong about this. Throwing an exception or printing a warning/error
> message would be better options.

The assertions here are not targeted at add-on developers, but the fact that the SDK team caught them very soon shows that they work as designed.  :-)
(In reply to Ehsan Akhgari from comment #29)
> (In reply to comment #27)
> > (From update of attachment 636714 [details] [diff] [review])
> > >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> > >--- a/browser/base/content/nsContextMenu.js
> > >+++ b/browser/base/content/nsContextMenu.js
> > >@@ -1117,7 +1117,7 @@ nsContextMenu.prototype = {
> > > 
> > >     var clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> > >                     getService(Ci.nsIClipboardHelper);
> > >-    clipboard.copyString(addresses);
> > >+    clipboard.copyString(addresses, document);
> > So, when you say per-window private browsing, you mean per chrome window?
> > (Otherwise I would have expected this to use a content document.)
> The platform level feature supports this flag per docshell.
So is there a reason this isn't using the docshell of the link being copied?

> (In reply to neil@parkwaycc.co.uk from comment #28)
> > (In reply to Josh Matthews from comment #1)
> > > * add an nsILoadContext member to nsTransferable, and Init(nsILoadContext*)
> > Although this turns out to be really annoying to do from chrome, as can be
> > seen by the need for helper functions everywhere. Slightly more useful would
> > have been an nsISupports* parameter, and then use do_GetInterface on that to
> > retrieve the load context.
> It's not that annoying, see nsIDocument::GetLoadContext.
I said chrome, not C++! Maybe there should have been a C++ version that takes an nsIDocument and a scriptable version that takes an nsIDOMDocument.

> > > * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
> > Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never
> > crash, even if it's only in debug builds. If you don't think non-fatal
> > assertions would get noticed, why not throw an exception?
> Transferable objects which have not been initialized are not a valid state
> of the program any more.  Unfortunately XPCOM doesn't provide us with a
> better way of ensuring this.
NS_ENSURE_STATE works for me.
(In reply to neil@parkwaycc.co.uk from comment #32)
> (In reply to Ehsan Akhgari from comment #29)
> > (In reply to comment #27)
> > > (From update of attachment 636714 [details] [diff] [review])
> > > >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> > > >--- a/browser/base/content/nsContextMenu.js
> > > >+++ b/browser/base/content/nsContextMenu.js
> > > >@@ -1117,7 +1117,7 @@ nsContextMenu.prototype = {
> > > > 
> > > >     var clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> > > >                     getService(Ci.nsIClipboardHelper);
> > > >-    clipboard.copyString(addresses);
> > > >+    clipboard.copyString(addresses, document);
> > > So, when you say per-window private browsing, you mean per chrome window?
> > > (Otherwise I would have expected this to use a content document.)
> > The platform level feature supports this flag per docshell.
> So is there a reason this isn't using the docshell of the link being copied?

It shouldn't make a difference.  If anything, we might want to use the content document.   Feel free to file a bug on that.

> > (In reply to neil@parkwaycc.co.uk from comment #28)
> > > (In reply to Josh Matthews from comment #1)
> > > > * add an nsILoadContext member to nsTransferable, and Init(nsILoadContext*)
> > > Although this turns out to be really annoying to do from chrome, as can be
> > > seen by the need for helper functions everywhere. Slightly more useful would
> > > have been an nsISupports* parameter, and then use do_GetInterface on that to
> > > retrieve the load context.
> > It's not that annoying, see nsIDocument::GetLoadContext.
> I said chrome, not C++! Maybe there should have been a C++ version that
> takes an nsIDocument and a scriptable version that takes an nsIDOMDocument.

This could be a follow-up bug as well.

> > > > * assert in that all nsITransferable methods that Init has been called (a null privacy context is ok, but we want to make it explicit)
> > > Regular NS_ASSERTIONs are fine, but IMHO scriptable methods should never
> > > crash, even if it's only in debug builds. If you don't think non-fatal
> > > assertions would get noticed, why not throw an exception?
> > Transferable objects which have not been initialized are not a valid state
> > of the program any more.  Unfortunately XPCOM doesn't provide us with a
> > better way of ensuring this.
> NS_ENSURE_STATE works for me.

I'm not sure I agree.
I just got the following addon incompatibility alert for my FF plugin, which is awesome:

"Error: The `nsITransferable` interface has changed to better support Private Browsing Mode. After instantiating the object, you should call `.init(null)` on it, before any other functions are called.

Error: See bug https://bugzilla.mozilla.org/show_bug.cgi?id=722872 for more information."

However, it raises two issues I wanted to bring up here, because this bug was referenced, and I wasn't sure who else to contact. The docs that I referenced in using nsITransferable are now out-of-date:
https://developer.mozilla.org/en-US/docs/Using_the_Clipboard

Also, the link on that page for the interface nsITransferable docs is currently broken, so I cannot tell if adding the call to .init(null) is backwards compatible with FF < 16. Any guidance is appreciated.
Jorge, that proposed solution for the compatibility warning is completely undermining the purpose of this API change; who do I talk to about correcting it? 

Thank you for bringing this up, Justin. The init method does not exist in versions <16, so you'll need to check for its existence first. Furthermore, null should only be passed in specific situations. If the transferable object is being used to store clipboard data, it _must_ be initialized with the load context that can be obtained from the source window (getInterface to nsIWebNavigation, QI the result to nsILoadContext).
I have updated https://developer.mozilla.org/en-US/docs/Using_the_Clipboard according to the API changes here. Please let me know if anything remains unclear.
Josh, thanks for the swift response. I discovered that the init method didn't exist (testing on FF 15) so added a simple typeof check (https://github.com/justincwatt/copy-as-html-link/commit/6d77791aeea63d63a9b707f1c53d25c6c318bbd5). However your "Using the Clipboard" update is much more thorough, so I'll update my plugin accordingly before resubmitting it. 

Quick question on the docs: would it make more sense to make the "var privacyContext = ..." part of the "if ('init' in trans)" block?
I filed bug 799247 to make this correction.
Josh, I updated my plugin based on your edits to https://developer.mozilla.org/en-US/docs/Using_the_Clipboard and now I'm getting the following error (using FF 15.0.1):

An unknown error occurred
ReferenceError: sourceWindow is not defined

It's unclear in the sample code where/how sourceWindow is instantiated.
Justin, the problem is that the new API requires a bit more thought on the part of the developer, and I welcome any suggestions (or edits) for how to make the docs clearer. The sourceWindow variable is the window that is the source of the data being placed in the transferable object, and must be provided by the developer.
Josh, some examples of (or links to) how one might instantiate sourceWindow would help (Googling for it turned up blanks). In my case, my plugin (Copy as HTML Link) adds an entry to the right-click context menu when text is selected, manipulates that next (not unlike the example in the docs) and copies the manipulated text to the clipboard.

If it helps to take a look, the code is here:
https://github.com/justincwatt/copy-as-html-link/blob/master/chrome/content/copy_as_html_link.js#L58
Justin, there is no instantiation necessary. If the data being placed on the clipboard comes from a content window, sourceWindow should just be that window object (ie. someDocument.defaultView if you have a content document object named someDocument).
In your case, it looks like you would want document.commandDispatcher.focusedWindow, since that is the content window from which the location is being copied.
Thanks, very helpful, makes perfect sense. As far as documentation goes, anything that would help someone not steeped in the arcana of Firefox (like me) make the conceptual leap from "sourceWindow" to, in my case, "document.commandDispatcher.focusedWindow" would probably be helpful. Perhaps some examples of typical source windows...?
I second Justin's suggestion in comment 44: a list of examples of typical source windows would really be helpful. I had to dig through the comments of this example to figure out how to implement things correctly.
Depends on: 826479
I still ran into this while following the MDN page for using the clipboard, so I made some further clarifications on that page. I'll file a bug for adding documentation for nsITransferable, since if this is needed for something basic as reading from the clipboard, it would be good if some documentation would explain this class.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: