773760 - Cross-process clipboard setting doesn't reflect privacy status of original transferable

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

ContentParent::RecvSetClipboard just creates a transferable with a blank privacy context. We should pass the privacy information from the original one over the wire and stuff that into the new transferable instead.

Comment 1

[(no longer active)]

Is this something you wanna take on Josh?

Comment 2

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/file/b5ae446888f5/dom/ipc/ContentParent.cpp#l689 - this is the receiver for http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#282, and is sent from http://mxr.mozilla.org/mozilla-central/source/widget/android/nsClipboard.cpp#53. We need to obtain http://mxr.mozilla.org/mozilla-central/source/widget/nsITransferable.idl#175, send it in the SetClipboardText message, and add a special method to http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsTransferable.cpp (and the nsITransferable IDL) that allows us to set the privacy status of a transferable object with just a boolean argument.

Comment 3

[Eric Wong (:ekw)]

Attachment: I'd like to try and fix this bug; please assign to me. This is my first go at a patch for this bug. Please let me know if I'm on the right track.

I'm confused though why nsITransferable.idl doesn't have the existing method nsTransferable::GetIsPrivateData() defined in it, so I didn't include the new method I added, nsTransferable::SetIsPrivateData(), yet.

Also, how would I test this?  Do I need to compile Firefox for Android and test on an android phone/tablet?  My patch is based entirely on the excellent explanation in comment 2 without having tried to reproduce the bug.

Comment 4

[Josh Matthews [:jdm]]

There is no way to test it, unfortunately. A) this code only runs in XUL Fennec, and B) XUL Fennec doesn't have a private browsing mode. However, this may end up running in B2G one day, and then we'll all be glad we fixed it.

Comment 5

[Josh Matthews [:jdm]]

Also, IDL attribute foo turns into GetFoo in C++ (and SetFoo if not readonly).

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 651266 [details] [diff] [review]
I'd like to try and fix this bug; please assign to me.  This is my first go at a patch for this bug.  Please let me know if I'm on the right track.

Great! This looks fine to me, but I'll get an official word from ehsan.

Comment 7

[(no longer active)]

Comment on attachment 651266 [details] [diff] [review]
I'd like to try and fix this bug; please assign to me.  This is my first go at a patch for this bug.  Please let me know if I'm on the right track.

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

Hmm, I don't particularly like the addition of the SetIsPrivateData method, but I see no better way of doing this...  :(

I'm mostly minusing the review because of the uuid change for nsITransferable.  My other comment is just a small nit.  Thanks for working on this!

::: widget/nsITransferable.idl
@@ +171,5 @@
>    void removeDataFlavor ( in string aDataFlavor ) ;
>  
>    attribute nsIFormatConverter converter;
>  
> +  [noscript] attribute boolean isPrivateData;

You need to change the uuid of the nsITransferable interface.  You can use http://www.famkruithof.net/uuid/uuidgen to generate a new uuid.

Also, please add a comment to isPrivateData saying that people should try to avoid calling the SetIsPrivateData method as much as they can...

Comment 8

[Eric Wong (:ekw)]

Should I put a reason in the comments for avoiding use of SetIsPrivateData()?  What's the reason?

Comment 9

[Josh Matthews [:jdm]]

The reason is that it overrides the privacy status with a value that may not reflect the status of the context in which the transferable was created.

Comment 10

[Eric Wong (:ekw)]

Attachment: Incorporate review comments.

Comment 11

[(no longer active)]

Comment on attachment 651641 [details] [diff] [review]
Incorporate review comments.

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

Looks great, thanks!

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/327d9e9efbf7

Comment 13

[Ed Morley [:emorley]]

Sorry, had to back out for build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=327d9e9efbf7

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14372384&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14372342&tree=Mozilla-Inbound

jdm, please can you help Eric get this fixed up and submit the patch to Try? :-)

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a7377ac153

Comment 14

[Eric Wong (:ekw)]

Argh, I see the problem.  Will fix and upload patch and ask for push to try.

Comment 15

[Eric Wong (:ekw)]

Attachment: Fix bustage in previous patch.

Please push to try.  Realize now my local compiles wouldn't have caught previous patch's bustage because I'm not set up to compile for android.

Comment 16

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=e359c1cfb406

Sorry we didn't do this the first time round!

Comment 17

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3bc014ea9703

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bc014ea9703