Closed Bug 773760 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jdm, Assigned: ekw)

References

Details

(Whiteboard: [mentor=jdm][lang=c++])

Attachments

(1 file, 2 obsolete files)

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.
Is this something you wanna take on Josh?
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.
Whiteboard: [mentor=jdm][lang=c++]
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.
Attachment #651266 - Flags: feedback?(josh)
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.
Also, IDL attribute foo turns into GetFoo in C++ (and SetFoo if not readonly).
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.
Attachment #651266 - Flags: review?(ehsan)
Attachment #651266 - Flags: feedback?(josh)
Attachment #651266 - Flags: feedback+
Assignee: nobody → ewong3
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...
Attachment #651266 - Flags: review?(ehsan) → review-
Should I put a reason in the comments for avoiding use of SetIsPrivateData()?  What's the reason?
Status: NEW → ASSIGNED
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.
Attached patch Incorporate review comments. (obsolete) — Splinter Review
Attachment #651266 - Attachment is obsolete: true
Attachment #651641 - Flags: review?(ehsan)
Comment on attachment 651641 [details] [diff] [review]
Incorporate review comments.

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

Looks great, thanks!
Attachment #651641 - Flags: review?(ehsan) → review+
Argh, I see the problem.  Will fix and upload patch and ask for push to try.
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.
Attachment #651641 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=e359c1cfb406

Sorry we didn't do this the first time round!
https://hg.mozilla.org/mozilla-central/rev/3bc014ea9703
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.