The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: ekw)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 2

5 years ago
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++]
(Assignee)

Comment 3

5 years ago
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.
Attachment #651266 - Flags: feedback?(josh)
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

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

Comment 6

5 years ago
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+
(Reporter)

Updated

5 years ago
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-
(Assignee)

Comment 8

5 years ago
Should I put a reason in the comments for avoiding use of SetIsPrivateData()?  What's the reason?
Status: NEW → ASSIGNED
(Reporter)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Created attachment 651641 [details] [diff] [review]
Incorporate review comments.
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/327d9e9efbf7
Target Milestone: --- → mozilla17
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
(Assignee)

Comment 14

5 years ago
Argh, I see the problem.  Will fix and upload patch and ask for push to try.
(Assignee)

Comment 15

5 years ago
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.
Attachment #651641 - Attachment is obsolete: true
(Reporter)

Comment 16

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

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

Comment 17

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/3bc014ea9703
https://hg.mozilla.org/mozilla-central/rev/3bc014ea9703
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.