Clipboard data is incomplete when quitting right after copy image command

REOPENED
Assigned to

Status

()

P3
major
REOPENED
11 years ago
5 years ago

People

(Reporter: Brade, Assigned: mcs)

Tracking

({regression})

Trunk
mozilla1.9beta2
x86
Mac OS X
regression
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Using Gran Paradiso 3.0a8 (or 10/16/2007 nightly bits), I see a problem with the clipboard data being incomplete (only on Mac OS X and not a problem in FF2.0.0.8).

Steps to reproduce:
1) Load a web page with an image
2) Use the context menu to "Copy Image"
3) Quit within a second or two
4) Go to the Finder's Show Clipboard command, see that it is blank
Expectation: it should show the image

Using Clipboard Viewer (a sample app that comes with XCode tools) I can see that there are two content types on the clipboard.  The TIFF data looks fine.  For the PICT data, Clipboard Viewer reports "Clipboard contents changed."
Flags: blocking-firefox3?
As far as I know we do not put any PICT data on the clipboard at all. Perhaps the OS or Cocoa is doing that conversion for us.

Does Safari exhibit this behavior as well?
Blocking for investigation.
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: qawanted
Can you try this in Thunderbird too? If so it should be moved to Widget:Cocoa
(Reporter)

Comment 4

11 years ago
I have Thunderbird 2.0.0.6 and cannot find any way to trigger this command (Copy Image).  I doubt that it will have this bug since it is on the branch.

I believe that the problem is in the Cocoa clipboard code so moving it to Widget:Cocoa is fine with me (as long as it still blocks 1.9).

Try a Thunderbird 3 nightly, but yeah it's probably widget:cocoa.
Assignee: nobody → joshmoz
Component: OS Integration → Widget: Cocoa
Flags: blocking-firefox3+
Product: Firefox → Core
QA Contact: os.integration → cocoa
Flags: blocking1.9?
Thunderbird doesn't expose copy image contents at all, but if it did it would be doing exactly the same thing as Firefox, just calling goDoCommand('cmd_copyImageContents')
(Reporter)

Comment 7

11 years ago
Using Safari 2.0.4, I am not able to reproduce this problem.  There is some strangeness in that after I do a "Copy Image" in Safari the Finder's Clipboard window is blank and it shows "Clipboard contents: Text" along the bottom.  But the Clipboard Viewer app shows a lot of flavors, including PICT and TIFF that both appear to have valid data.
I don't see the "View Clipboard" problem in today's Camino trunk nightly, fwiw.  However, when I try to save the PICT data from Clipboard Viewer and open that in Preview, Preview won't open it (GraphicConverter opens it just fine, however, with no warnings about corruption).
(Assignee)

Comment 9

11 years ago
I also was unable to reproduce this problem with the latest Camino nightly build.  I wonder why Camino does not have this problem?

The PasteboardDictFromTransferable() method in nsClipboard.mm only places TIFF data on the clipboard for image copies.  Some other code must convert the TIFF to PICT.

I did two additional experiments:

1) Changed the code in nsClipboard.mm to convert to PICT instead of TIFF and place PICT on the clipboard.  Result: Only PICT data on the clipboard, no strange behavior.

2) Changed the code in nsClipboard.mm to convert to PICT in addition to TIFF and place both flavors on the clipboard.  Result: TIFF and PICT data on the clipboard, no strange behavior.

I tried looking for a system component that might be converting from TIFF to PICT but came up empty (I am not sure how to find all of the filters though).
We should probably do #2, IMO. It's probably something inside WindowServer that is doing the conversion, perhaps for compatibility with old school Clipboard Manager stuff?

Should be pretty easy, do you have a patch?

Additionally, is this bug reproducible on 10.5?
(Assignee)

Comment 11

11 years ago
Created attachment 286682 [details] [diff] [review]
proposed fix

Here is the patch.  This has been tested on Mac OS 10.4.10 Intel with my Firefox trunk build.  I do not have 10.5 installed yet.
Assignee: joshmoz → mcs
Status: NEW → ASSIGNED
Attachment #286682 - Flags: review?(cbarrett)
Comment on attachment 286682 [details] [diff] [review]
proposed fix

First off, thanks for the patch!


>       if (destRef)

Let's rename this to tiffDestRef.

>+      // Also convert to PICT data.  We do this to work around a problem with
>+      // a system component that places invalid data PICT data on the
>+      // when it sees TIFF data.  See bug 400028.

Looks like you stopped in the middle of typing and started again. Also, make it clear that this bug doesn't happen all the time. (For bonus points, make a reduced test case and file a bug with Apple :)

>+      PRBool pictSuccessfullyConverted = CGImageDestinationFinalize(pictDestRef);

I am not sure if we want to have a separate state for this. If we fail at getting PICT data, shouldn't we just bail?

>+      if (pictSuccessfullyConverted)
>+      {
>+          [pasteboardOutputDict setObject:(NSMutableData*)pictData forKey:NSPICTPboardType];
>+      }
>+      if (pictData)
>+        CFRelease(pictData);

|if (foo) {|, please. The else's below are wrong, should be |} else|, but you don't have to clean that up if you don't want to.


I'll hopefully be able to test this on 10.5.0 soon.
Attachment #286682 - Flags: review?(cbarrett) → review-
(Assignee)

Comment 13

11 years ago
Created attachment 286725 [details] [diff] [review]
proposed fix, v2

Here is a revised patch that addresses the review comments.  I think it is debatable whether we should bail if the conversion to PICT fails.  I assume it won't fail unless something goes terribly wrong though, so I changed the code to just continue in that case.
Attachment #286682 - Attachment is obsolete: true
Attachment #286725 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #286725 - Flags: review? → review?(cbarrett)
Comment on attachment 286725 [details] [diff] [review]
proposed fix, v2

Yeah, by "bail" I meant continue.

>-    }
>-    else if (flavorStr.EqualsLiteral(kPNGImageMime) || flavorStr.EqualsLiteral(kJPEGImageMime) ||
>+    } else if (flavorStr.EqualsLiteral(kPNGImageMime) || flavorStr.EqualsLiteral(kJPEGImageMime) ||

OK it turns out I was wrong, module style's the way it was originally.

>+      CGImageDestinationAddImage(tiffDestRef, imageRef, NULL);
>+      PRBool tiffSuccessfullyConverted = CGImageDestinationFinalize(tiffDestRef);
>+      if (tiffDestRef)
>+        CFRelease(tiffDestRef);
>+

If it were me, I'd drop "sucessfully", but it doesn't really matter.

>+      if (NS_FAILED(image->UnlockImagePixels(PR_FALSE))
>+          || !tiffSuccessfullyConverted
>+          || !pictSuccessfullyConverted) {

Module style is definitely that operators go at the end of the line.

I also would do it that either both conversions work or neither do, but I don't think it matters so much.

r=me with those style changes.

Since I'm not a peer and can't do reviews, asking josh for review.
Attachment #286725 - Flags: review?(joshmoz)
Attachment #286725 - Flags: review?(cbarrett)
Attachment #286725 - Flags: review+
(Assignee)

Comment 15

11 years ago
Created attachment 286869 [details] [diff] [review]
proposed fix, v3

Here is v3 of the patch, revised to account for review comments.
Attachment #286725 - Attachment is obsolete: true
Attachment #286869 - Flags: review?
Attachment #286725 - Flags: review?(joshmoz)
(Assignee)

Updated

11 years ago
Attachment #286869 - Flags: review? → review?(joshmoz)

Comment 16

11 years ago
Can anyone reproduce this bug on 10.5 final? Also, if this is a bug in Mac OS X I'd like to see an Apple bug filed.

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+

Comment 17

11 years ago
Comment on attachment 286869 [details] [diff] [review]
proposed fix, v3

This code looks fine, but please file a bug with Apple and record in this bug whether or not the bug can be reproduced on Mac OS X 10.5.

Thanks for tracking this down!
Attachment #286869 - Flags: superreview?(roc)
Attachment #286869 - Flags: review?(joshmoz)
Attachment #286869 - Flags: review+
Since the PICT and TIFF code looks basically identical except for the type CFSTR and the pasteboardOutputDict forKey, can't we have a single function to do the work and just call it twice?
(In reply to comment #16)
> Can anyone reproduce this bug on 10.5 final? Also, if this is a bug in Mac OS X
> I'd like to see an Apple bug filed.

I can reproduce comment 0 (using an October 25 Minefield) and comment 8 on 10.5 final.

(Assignee)

Comment 20

11 years ago
Created attachment 287548 [details] [diff] [review]
proposed fix, v4 (added function)

One more time, with AddImageDataToPasteboardDict() helper function.
Attachment #286869 - Attachment is obsolete: true
Attachment #287548 - Flags: review?(joshmoz)
Attachment #286869 - Flags: superreview?(roc)

Comment 21

11 years ago
Comment on attachment 287548 [details] [diff] [review]
proposed fix, v4 (added function)

+      // component that places invalid data PICT data on the clipboard when

On checkin, please remove the extra instance of "data" in that comment.
Attachment #287548 - Flags: superreview?(roc)
Attachment #287548 - Flags: review?(joshmoz)
Attachment #287548 - Flags: review+
Comment on attachment 287548 [details] [diff] [review]
proposed fix, v4 (added function)

+  CGImageDestinationAddImage(destRef, aSourceImageRef, NULL);
+  PRBool imageConverted = CGImageDestinationFinalize(destRef);
+  if (destRef)

So those two functions are safe to call with null destRef?
Attachment #287548 - Flags: superreview?(roc) → superreview+

Comment 23

11 years ago
I believe it is safe. NULL propagation is OK in the CG APIs, iirc.

Comment 24

11 years ago
Mark - do you have cvs commit privs to land this when the tree is open for M10?
(Assignee)

Comment 25

11 years ago
(In reply to comment #24)
> Mark - do you have cvs commit privs to land this when the tree is open for M10?

Yes.

Updated

11 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.9 M10
(Assignee)

Comment 26

11 years ago
Committed to the trunk:

mozilla/widget/src/cocoa/nsClipboard.mm
new revision: 1.26; previous revision: 1.25
  Fix problem where PICT clipboard data is incomplete if you quit
  immediately after copying an image.  Bug 400028.  r=joshmoz,sr=roc,a=joshmoz.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 27

11 years ago
This fix causes drags to stop working; see bug 407020.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Blocks: 407020

Comment 28

11 years ago
Since this is less serious than the bug it causes, we'll have to back this out if we don't have a fix soon.

Comment 29

11 years ago
until this is actually backed out and it becomes a problem again, this bug should remain fixed
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 30

11 years ago
Created attachment 301577 [details] [diff] [review]
Revert the patch

This patch reverts the change, necessary because of the serious regression observed in bug 407020. A future candidate fix should be tested for both cut-n-paste and dragging behavior, since both kinds of operations use the clipboard.
Attachment #287548 - Attachment is obsolete: true
Attachment #301577 - Flags: superreview?
Attachment #301577 - Flags: review?(joshmoz)
Flags: in-litmus?

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Attachment #301577 - Flags: superreview?(roc)
Attachment #301577 - Flags: superreview?
Attachment #301577 - Flags: review?(joshmoz)
Attachment #301577 - Flags: review+
Attachment #301577 - Flags: superreview?(roc) → superreview+

Updated

11 years ago
Keywords: checkin-needed

Comment 31

11 years ago
Blocking status for this is a tough call, I'm going to go with blocking1.9- for now but it would be really nice to have a fix. The backout can still land without approval for now since it fixes blocking bug 407020.
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+

Comment 32

11 years ago
backed out
Keywords: checkin-needed
The qawanted tag was added for investigation this was investigated fixed and backed out in the final so the tag is not actual anymore. Removing qawanted.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.