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."

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.

Can you try this in Thunderbird too? If so it should be moved to Widget:Cocoa

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.

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')

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).

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?

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.

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.

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.

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.

Here is v3 of the patch, revised to account for review comments.

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.

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!

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.

One more time, with AddImageDataToPasteboardDict() helper function.

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.

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?

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

Mark - do you have cvs commit privs to land this when the tree is open for M10?

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

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.

This fix causes drags to stop working; see bug 407020.

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.

until this is actually backed out and it becomes a problem again, this bug should remain fixed

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.

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.

backed out

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.