400028 - Clipboard data is incomplete when quitting right after copy image command

Status: RESOLVED WORKSFORME

(Core :: Widget: Cocoa, defect, P3)

Description

[Kathleen :Brade]

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

Comment 1

[Colin Barrett [:cbarrett]]

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?

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for investigation.

Comment 3

[Colin Barrett [:cbarrett]]

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

Comment 4

[Kathleen :Brade]

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

Comment 5

[Colin Barrett [:cbarrett]]

Try a Thunderbird 3 nightly, but yeah it's probably widget:cocoa.

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Kathleen :Brade]

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.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 9

[Mark Smith [:mcs]]

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

Comment 10

[Colin Barrett [:cbarrett]]

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?

Comment 11

[Mark Smith [:mcs]]

Attachment: 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.

Comment 12

[Colin Barrett [: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.

Comment 13

[Mark Smith [:mcs]]

Attachment: 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.

Comment 14

[Colin Barrett [: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.

Comment 15

[Mark Smith [:mcs]]

Attachment: proposed fix, v3

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

Comment 16

[Josh Aas]

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 17

[Josh Aas]

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!

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 20

[Mark Smith [:mcs]]

Attachment: proposed fix, v4 (added function)

One more time, with AddImageDataToPasteboardDict() helper function.

Comment 21

[Josh Aas]

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 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 23

[Josh Aas]

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

Comment 24

[Josh Aas]

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

Comment 25

[Mark Smith [:mcs]]

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

Yes.

Comment 26

[Mark Smith [:mcs]]

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.

Comment 27

[Stan Shebs]

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

Comment 28

[Josh Aas]

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

[Josh Aas]

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

Comment 30

[Stan Shebs]

Attachment: 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.

Comment 31

[Josh Aas]

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.

Comment 32

[Josh Aas]

backed out

Comment 33

[Catalin Varga [QA][:VarCat]]

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.

Comment 34

[Alin Ilea, Desktop QA]

Hi,

I have tested your issue on latest Nightly build 96.0a1 (2021-11-04) and could not reproduce it using macOS 10.15 following the steps from the description.
@Kathleen is the issue still reproducible on your end or it is not relevant anymore and it can be closed?

Thanks.

Comment 35

[Kathleen :Brade]

Apparently this bug was fixed at some point. :-)