Last Comment Bug 773760 - Cross-process clipboard setting doesn't reflect privacy status of original transferable
: Cross-process clipboard setting doesn't reflect privacy status of original tr...
Status: RESOLVED FIXED
[mentor=jdm][lang=c++]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Eric Wong (:ekw)
:
Mentors:
Depends on:
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-07-13 12:06 PDT by Josh Matthews [:jdm]
Modified: 2012-08-15 18:46 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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. (5.76 KB, patch)
2012-08-12 22:10 PDT, Eric Wong (:ekw)
ehsan: review-
josh: feedback+
Details | Diff | Splinter Review
Incorporate review comments. (6.78 KB, patch)
2012-08-13 22:57 PDT, Eric Wong (:ekw)
ehsan: review+
Details | Diff | Splinter Review
Fix bustage in previous patch. (6.79 KB, patch)
2012-08-14 20:43 PDT, Eric Wong (:ekw)
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-07-13 12:06:34 PDT
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 :Ehsan Akhgari 2012-07-16 14:16:04 PDT
Is this something you wanna take on Josh?
Comment 2 Josh Matthews [:jdm] 2012-08-12 13:04:42 PDT
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) 2012-08-12 22:10:42 PDT
Created 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.

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] 2012-08-12 22:38:33 PDT
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] 2012-08-12 22:39:09 PDT
Also, IDL attribute foo turns into GetFoo in C++ (and SetFoo if not readonly).
Comment 6 Josh Matthews [:jdm] 2012-08-12 22:42:17 PDT
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 :Ehsan Akhgari 2012-08-13 10:40:23 PDT
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) 2012-08-13 19:25:15 PDT
Should I put a reason in the comments for avoiding use of SetIsPrivateData()?  What's the reason?
Comment 9 Josh Matthews [:jdm] 2012-08-13 20:40:57 PDT
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) 2012-08-13 22:57:30 PDT
Created attachment 651641 [details] [diff] [review]
Incorporate review comments.
Comment 11 :Ehsan Akhgari 2012-08-14 07:51:45 PDT
Comment on attachment 651641 [details] [diff] [review]
Incorporate review comments.

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

Looks great, thanks!
Comment 14 Eric Wong (:ekw) 2012-08-14 20:24:53 PDT
Argh, I see the problem.  Will fix and upload patch and ask for push to try.
Comment 15 Eric Wong (:ekw) 2012-08-14 20:43:33 PDT
Created attachment 651985 [details] [diff] [review]
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] 2012-08-14 21:56:53 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e359c1cfb406

Sorry we didn't do this the first time round!
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:46:40 PDT
https://hg.mozilla.org/mozilla-central/rev/3bc014ea9703

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