Closed Bug 664717 Opened 13 years ago Closed 8 years ago

"copy image" to copy a animated GIF will only copy one frame of it

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
4.0 Branch
Platform:
x86
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: dindog, Assigned: hectorz)

References

Details

Attachments

(4 files, 5 obsolete files)

demo copy gif difference between IE & Fx
462.07 KB, image/gif
Details
Bug 664717 - Part 1: Switch to requestingPrincipal in nsITransferable.
58 bytes, text/x-review-board-request
ckerschb
: review+
Details
Bug 664717 - Part 2: Pass requestingPrincipal to parent process with nsITransferable.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 664717 - Part 3: Include file promise when copying an image in Windows.
58 bytes, text/x-review-board-request
seth
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0

use "copy image" in the content menu to copy a animated GIF will only got one frame of the animation. If you select it by draging (it will be highlight), then Ctrl+C, paste it to other application is normal animation. So I'm curious, Firefox behaves that way on purpose?

it's been long time, at least Fx3.6, so probably there are duplicated bugs, but just in case, so decide to file a new one.

Reproducible: Always



Expected Results:  
copy an animation should got an animation in clipboard
What else would it do? I don't think the OS clipboards support animations. Can you give an example of where this works between 2 different apps?
I was able to copy (by context menu) and paste to desktop this image bellow in IE9, but Firefox give nothing (paste menu disabled on desktop).

http://upload.wikimedia.org/wikipedia/commons/5/50/Triple-Spiral-Labyrinth-animated.gif

Bug confirmed?
(In reply to comment #1)
> What else would it do? I don't think the OS clipboards support animations.
> Can you give an example of where this works between 2 different apps?

I capture a video how IE & Firefox behave when copy a GIF. (IE 8.0 & Fx Nightly)

First, copy the gif, paste in a Instant Messager which allow user send Gif, then paste it in Photoshop.

Then repeat, but use "copy image" in Fx

In Photoshop, both IE & Fx paste a frame: IE paste the beginning, Fx paste the frame was shown when "copy image" clicked.

In IM, IE's clipboard paste a animation, Fx still one frame.


IE way is better

    
    

    
  
Tested Chrome and Opera, the same as Fx, no wonder you think OS clipboards don't support animations.

IE outplay others on this, unless you still think Fx mechanism is better.

    
  
Version: unspecified → 4.0 Branch

    
    

    
  
a friend of a forum make a more sophisticated test, using a simple c# application.

When you copy selecte something in a web page contain font color, size and link, there are two different kinds of text: plain text and with style, and that is true both in Fx and IE.


For example, copy the above comment 5, we got plain text:
___________________________________________________________________________
dindog@163.com 2011-06-17 21:39:20 PDT

Tested Chrome and Opera, the same as Fx, no wonder you think OS clipboards don't support animations.

IE outplay others on this, unless you still think Fx mechanism is better.
*******************************************************************************

and rich text:
______________________________________________________________________________
Version:0.9
StartHTML:00000160
EndHTML:00000836
StartFragment:00000194
EndFragment:00000800
SourceURL:https://bugzilla.mozilla.org/show_bug.cgi?id=664717
<html><body>
<!--StartFragment--><div class="bz_comment_head"><span class="bz_comment_user"><span class="vcard"><a class="email" href="mailto:dindog@163.com" title="dindog@163.com">dindog@163.com</a>
</span>
        </span>

        <span class="bz_comment_user_images">
        </span>

        <span class="bz_comment_time">
          2011-06-17 21:39:20 PDT
        </span>
      </div>



<pre class="bz_comment_text" id="comment_text_5">Tested Chrome and Opera, the same as Fx, no wonder you think OS clipboards don't support animations.

IE outplay others on this, unless you still think Fx mechanism is better.</pre><!--EndFragment-->
</body>
</html>
_______________________________________________________________________________

though the rich text format is quite different.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
BUT come to copy image, IE always got three element and Fx has only one. 

Fx only copy to clipboard a bitmap

IE has rich text, file and bitmap, which is convenient in many scenario

    
    

    
  
Hmm, interesting. I'd expect it's the file flavor in the clipboard that's making this work. Well, unless RTF is making it work by having the app it's pasted into load the image from the web. Seems reasonable, and would be nice to just combine "Copy Image" and "Copy Image Location" at some point too.

The relevant code is in http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCopySupport.cpp, nsCopySupport::ImageCopy(). It should be possible to get the file data from the nsIImageLoadingContent, though offhand I'm not sure how.
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
Support. The IE way is much more convenient, especially when share animation pictures from web to your friend.

    
    

    
      
  

    
    

    
  
Please don't add "me too!" comments that only create bugmail. Providing patches helps to get things forward. Also see https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

    
    

    
  

      
      
      
      

        Attached image
          
        clipboard-IE.png (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        clipboard-firefox.png (obsolete)
        — 
      

    


  

    
    

    
  
There is a tool "insideclipboard" which can show the content in clipboard, you can get it on http://www.nirsoft.net/utils/inside_clipboard.html

After copying an image in IE and firefox, for this bug, the difference seems that:
whether clipboard's contains CF_HDROP format data, see attachments.

The problem maybe firefox dose not support CF_HDROP when copy image.

    
    

    
  
Add three lines in widget\windows\nsClipboard.cpp::SetupNativeDataObject after comment "Add DIBv3" 
// Add HDROP
SET_FORMATETC(imageFE, CF_HDROP, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL)
dObj->AddDataFlavor(flavorStr, &imageFE);

It will support CF_HDROP, see attachment.

But, the drag and drop file is ".bmp" format, the img has been converted to a bitmap,
See widget\windows\nsDataOb.cpp::DropImage

So, for this bug, the img file should be saved first and then set HDROP file path to it.

But how can we save the img file? the bitmap file is created from render engine(after img file decoded),
how can we restore and save the img file after it has been decoded in render engine? or any other approaches?

Anyone can give some clues?

    
    

    
  

      
      
      
      

        Attached image
          
        clipboard-firefox-support-hdrop.png (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch, proof of concept (obsolete)
        — 
          — Splinter Review
      

    


  
With this patch, I can copy an animated gif into Tencent QQ on Windows.

Something I'm not quite sure of:
* I tried to set the |aImageElement| QI-ed into nsIDOMNode as the requestingNode of the nsITransferable, but still failed the assertion at [1]. The requestingNode part was introduced in bug 1038756. After reading comments of NS_NewChannel[2], I think in the case of nsITransferable, where the file data loaded from internet/cache will not be used in any document but saved as a local file with drag/drop or copy/paste, it's not really necessary to use the loadingDocument, I just removed the check for this patch. :ckerschb, is that acceptable?
* The logic to generate imageSourceString & imageDestFileName was copied from [3], and adding kFilePromiseMime flavor without setting a nsIFlavorDataProvider will not work on OSX[4]. Would it be proper if I move the common parts into nsContentUtils?
* Also I'm not sure how to organize the additional |#include|

[1]: https://dxr.mozilla.org/mozilla-central/rev/38c1ea9ccae3/widget/windows/nsDataObj.cpp#343
[2]: https://dxr.mozilla.org/mozilla-central/rev/38c1ea9ccae3/netwerk/base/nsNetUtil.h#96
[3]: https://dxr.mozilla.org/mozilla-central/rev/38c1ea9ccae3/dom/base/nsContentAreaDragDrop.cpp?#555
[4]: https://dxr.mozilla.org/mozilla-central/rev/38c1ea9ccae3/dom/base/nsContentUtils.cpp#7492

        Attachment #8597037 -
        Attachment is obsolete: true

        Attachment #8597038 -
        Attachment is obsolete: true

        Attachment #8597057 -
        Attachment is obsolete: true

        Attachment #8646721 -
        Flags: feedback?(mozilla)

        Attachment #8646721 -
        Flags: feedback?(dolske)

    
    

    
  
Comment on attachment 8646721 [details] [diff] [review]
Patch, proof of concept

It's been eons since I worked in clipboard code, so I'm probably not the best person for feedback here. I'd suggest a DOM peer and/or Seth Fowler.

        Attachment #8646721 -
        Flags: feedback?(dolske)

    
    

    
  
Comment on attachment 8646721 [details] [diff] [review]
Patch, proof of concept

(In reply to Justin Dolske [:Dolske] from comment #17)
> 
> It's been eons since I worked in clipboard code, so I'm probably not the
> best person for feedback here. I'd suggest a DOM peer and/or Seth Fowler.

Requesting feedback from :baku (touches /dom/base/nsCopySupport.cpp recently and a DOM peer) and :seth per :Dolske's advice, thanks.

        Attachment #8646721 -
        Flags: feedback?(seth)

        Attachment #8646721 -
        Flags: feedback?(amarchesini)

    
    

    
  
Comment on attachment 8646721 [details] [diff] [review]
Patch, proof of concept

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

I'm not familiar with these gecko parts.

::: dom/base/nsCopySupport.cpp
@@ +571,5 @@
> +
> +  // Fix the file extension in the URL if necessary
> +  if (aImgRequest && mimeService) {
> +    nsCOMPtr<nsIURI> imgUri;
> +    aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));

this can fail.

@@ +577,5 @@
> +    nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri));
> +
> +    if (imgUrl) {
> +      nsAutoCString extension;
> +      imgUrl->GetFileExtension(extension);

this can fail.

@@ +580,5 @@
> +      nsAutoCString extension;
> +      imgUrl->GetFileExtension(extension);
> +
> +      nsXPIDLCString mimeType;
> +      aImgRequest->GetMimeType(getter_Copies(mimeType));

this too.

@@ +583,5 @@
> +      nsXPIDLCString mimeType;
> +      aImgRequest->GetMimeType(getter_Copies(mimeType));
> +
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      mimeService->GetFromTypeAndExtension(mimeType, EmptyCString(),

here too.

@@ +605,5 @@
> +
> +          imgUrl = do_QueryInterface(imgUri);
> +
> +          nsAutoCString primaryExtension;
> +          mimeInfo->GetPrimaryExtension(primaryExtension);

this can fail.

        Attachment #8646721 -
        Flags: feedback?(amarchesini)

    
    

    
  
Comment on attachment 8646721 [details] [diff] [review]
Patch, proof of concept

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

(In reply to Hector Zhao [:hectorz] from comment #16)
> Something I'm not quite sure of:
> * I tried to set the |aImageElement| QI-ed into nsIDOMNode as the
> requestingNode of the nsITransferable, but still failed the assertion at
> [1]. The requestingNode part was introduced in bug 1038756. After reading
> comments of NS_NewChannel[2], I think in the case of nsITransferable, where
> the file data loaded from internet/cache will not be used in any document
> but saved as a local file with drag/drop or copy/paste, it's not really
> necessary to use the loadingDocument, I just removed the check for this
> patch. :ckerschb, is that acceptable?

Hey Hector, unfortunately we can't accept that change. Please *do not* remove the bits for the requestingNode to be passed to NS_NewChannel().
I am not the expert for drag and drop operations [1], but is there no possibility to pass the node? Maybe you should consult with jmathies.
Using the systemPrincipal would cause all security checks to be bypassed for all of the drag and drop operations which is obviously not that great.

JFYI: we are working on moving security checks within nsDataObj.cpp over in Bug 1192945.

If you really can not query a requestingNode, then you would have to add some special do to the 'transferable' that accounts specificially for your case and pass the requestingPrincipal in that case.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsDragService.cpp#301

        Attachment #8646721 -
        Flags: feedback?(mozilla) → feedback-

    
    

    
  
Comment on attachment 8646721 [details] [diff] [review]
Patch, proof of concept

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

Only looking at AppendImagePromise(), because most of this patch is touching stuff that I have no familiarity with.

::: dom/base/nsCopySupport.cpp
@@ +27,5 @@
> +#include "nsIMIMEService.h"
> +#include "nsIURL.h"
> +#include "nsIMimeInfo.h"
> +#include "nsReadableUtils.h"
> +#include "nsEscape.h"

Nit: please alphabetize these new |#include|'s.

@@ +80,5 @@
>                                nsINode* aDOMNode);
>  
> +// copy image as file promise onto the transferable
> +static nsresult AppendImagePromise(nsITransferable *aTransferable,
> +                                   imgIRequest *aImgRequest);

Nit: |nsITransferable* aTransferable| and |imgIRequest* aImgRequest|, per Mozilla style.

Also, why don't you just define this up here? It's better to avoid forward declarations where possible.

@@ +558,5 @@
>    return AppendString(aTransferable, context, kHTMLContext);
>  }
>  
> +static nsresult AppendImagePromise(nsITransferable *aTransferable,
> +                                   imgIRequest *aImgRequest)

Nit: Fix the style here too.

@@ +563,5 @@
> +{
> +  nsresult rv;
> +
> +  nsString imageSourceString;
> +  nsString imageDestFileName;

Nit: don't declare variables at the top of functions. Declare them at their first use. The exception is |rv|, which you can declare at the top of the function if you prefer it.

@@ +569,5 @@
> +  nsCOMPtr<nsIMIMEService> mimeService =
> +    do_GetService("@mozilla.org/mime;1");
> +
> +  // Fix the file extension in the URL if necessary
> +  if (aImgRequest && mimeService) {

Don't put the whole body of this function inside an |if| block. If aImgRequest or mimeService is null, just return early.

@@ +573,5 @@
> +  if (aImgRequest && mimeService) {
> +    nsCOMPtr<nsIURI> imgUri;
> +    aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));
> +
> +    nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri));

Uhh, |imgUri| is already an nsIURI. Get rid of this line, and get rid of the |imgUrl| variable.

@@ +575,5 @@
> +    aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));
> +
> +    nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri));
> +
> +    if (imgUrl) {

Again, looks like you should return early if this fails.

@@ +586,5 @@
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      mimeService->GetFromTypeAndExtension(mimeType, EmptyCString(),
> +                                           getter_AddRefs(mimeInfo));
> +
> +      if (mimeInfo) {

Looks like another place where you should return early, right?

@@ +602,5 @@
> +          // Fix the file extension in the URL
> +          nsresult rv = imgUrl->Clone(getter_AddRefs(imgUri));
> +          NS_ENSURE_SUCCESS(rv, rv);
> +
> +          imgUrl = do_QueryInterface(imgUri);

See above. |imgUri| is already an nsIURI. Delete this line.

@@ +627,5 @@
> +                            kFilePromiseURLMime);
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +
> +        if (!imageDestFileName.IsEmpty()) {

Should this be an |else if|? Does it make sense to run both of these blocks?

        Attachment #8646721 -
        Flags: feedback?(seth) → feedback-

    
    

    
  
Hi all, thanks for all the feedbacks, especially for the extra efforts to point out my style problems since I'm not very familiar with C++ development.

(In reply to Andrea Marchesini (:baku) from comment #19)
> 
> I'm not familiar with these gecko parts.
> 
> ::: dom/base/nsCopySupport.cpp
> @@ +571,5 @@
> > +
> > +  // Fix the file extension in the URL if necessary
> > +  if (aImgRequest && mimeService) {
> > +    nsCOMPtr<nsIURI> imgUri;
> > +    aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));
> 
> this can fail.
> 
> ...

This part was mostly just copied from [1].

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> 
> Hey Hector, unfortunately we can't accept that change. Please *do not*
> remove the bits for the requestingNode to be passed to NS_NewChannel().
> I am not the expert for drag and drop operations [1], but is there no
> possibility to pass the node? Maybe you should consult with jmathies.

I tried using SetRequestingNode similar to [2], but it seems the weak reference doesn't live long enough for [3] to succeed.

> Using the systemPrincipal would cause all security checks to be bypassed for
> all of the drag and drop operations which is obviously not that great.
> 

I think security checks using this requestingNode will not happen for all the drag and drop operations, but only when drag an image or a link and drop it into Windows explorer to save as a local file?

(In reply to Seth Fowler [:seth] [:s2h] from comment #21)
> 
> Only looking at AppendImagePromise(), because most of this patch is touching
> stuff that I have no familiarity with.
> 
> ::: dom/base/nsCopySupport.cpp
> @@ +80,5 @@
> >                                nsINode* aDOMNode);
> >  
> > +// copy image as file promise onto the transferable
> > +static nsresult AppendImagePromise(nsITransferable *aTransferable,
> > +                                   imgIRequest *aImgRequest);
> 
> Nit: |nsITransferable* aTransferable| and |imgIRequest* aImgRequest|, per
> Mozilla style.
> 
> Also, why don't you just define this up here? It's better to avoid forward
> declarations where possible.

This was the result of my search-n-replace of AppendDOMNode.

> 
> @@ +569,5 @@
> > +  nsCOMPtr<nsIMIMEService> mimeService =
> > +    do_GetService("@mozilla.org/mime;1");
> > +
> > +  // Fix the file extension in the URL if necessary
> > +  if (aImgRequest && mimeService) {
> 
> Don't put the whole body of this function inside an |if| block. If
> aImgRequest or mimeService is null, just return early.

This was also the part copied from [1].

> 
> @@ +573,5 @@
> > +  if (aImgRequest && mimeService) {
> > +    nsCOMPtr<nsIURI> imgUri;
> > +    aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));
> > +
> > +    nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri));
> 
> Uhh, |imgUri| is already an nsIURI. Get rid of this line, and get rid of the
> |imgUrl| variable.

I think |imgUri| is QI-ed into nsIURL for |imgUrl->GetFileExtension| to work? Or do you mean |imgUri| is already an nsIURL?

> 
> @@ +627,5 @@
> > +                            kFilePromiseURLMime);
> > +          NS_ENSURE_SUCCESS(rv, rv);
> > +        }
> > +
> > +        if (!imageDestFileName.IsEmpty()) {
> 
> Should this be an |else if|? Does it make sense to run both of these blocks?

I think both blocks should be run. kFilePromiseDestFilename is kinda optional on Windows[4] but necessary on OSX[5].

[1]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df/dom/base/nsContentAreaDragDrop.cpp#546
[2]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df/widget/windows/nsDragService.cpp#216
[3]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df/widget/windows/nsDataObj.cpp#345
[4]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df/widget/windows/nsDataObj.cpp#2042
[5]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df/dom/base/nsContentAreaDragDrop.cpp#213

    
    

    
  
(In reply to Hector Zhao [:hectorz] from comment #22)
> This was also the part copied from [1].

That's fine, but you still need to fix it before you can land the patch. The patch, as-is, is quite hard to read, and the deep nesting is part of it.

> I think |imgUri| is QI-ed into nsIURL for |imgUrl->GetFileExtension| to
> work? Or do you mean |imgUri| is already an nsIURL?

Hah! You're right. I managed to misread |nsIURL| as |nsIURI|. My apologies. I guess my eyesight is going in my old age. =)

> I think both blocks should be run. kFilePromiseDestFilename is kinda
> optional on Windows[4] but necessary on OSX[5].

That seems reasonable. This sort of thing is why someone familiar with the drag-and-drop code should review; I think all the people giving feedback aren't familiar with this code.

    
    

    
  


  
Try picking this up again, requesting feedback from :jimm per :ckerschb's suggestion in comment #20.

This patch has nothing really new, mostly cleanup of the previous one, and I restored the `requestingNode` of `nsITransferable`.

(In reply to Hector Zhao [:hectorz] from comment #22)
> 
> I tried using SetRequestingNode similar to [2], but it seems the weak
> reference doesn't live long enough for [3] to succeed.

I found out it only fails the assertion at [1] with e10s on, so I guess this must be a result of not handling `requestingNode` within `nsClipboardProxy`[2]. I'm not sure how to get it fixed, is it even possible to pass a DOM node to chrome/parent process?

Also I think this will only work on Windows, since `kFilePromiseDirectoryMime` is only set during drag-n-drop on OS X[3].

[1]: https://dxr.mozilla.org/mozilla-central/rev/0a25833062a8/widget/windows/nsDataObj.cpp#347
[2]: https://dxr.mozilla.org/mozilla-central/rev/0a25833062a8/widget/nsClipboardProxy.cpp#31
[3]: https://dxr.mozilla.org/mozilla-central/rev/0a25833062a8/widget/cocoa/nsChildView.mm#5954

        Attachment #8646721 -
        Attachment is obsolete: true

        Attachment #8748525 -
        Flags: feedback?(jmathies)

    
    

    
  
Comment on attachment 8748525 [details] [diff] [review]
Patch, updated proof of concept, not working with e10s yet

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

Why was I asked for feedback here? All of this code is in dom.

        Attachment #8748525 -
        Flags: feedback?(jmathies)

    
  
Flags: needinfo?(bzhao)

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #25)
> 
> Why was I asked for feedback here? All of this code is in dom.

Both :ckerschb (comment 20) and :seth (comment 23) suggested I ask feedback from someone familiar with drag-and-drop, also my patch already works in non-e10s mode, I cannot figure out how to pass a DOM node between the processes to finish the patch.

I'm hoping you can help me with your experiences in both drag-and-drop and e10s, thanks!
Flags: needinfo?(bzhao)

    
    

    
  
(In reply to Hector Zhao [:hectorz] from comment #26)
> (In reply to Jim Mathies [:jimm] from comment #25)
> > 
> > Why was I asked for feedback here? All of this code is in dom.
> 
> Both :ckerschb (comment 20) and :seth (comment 23) suggested I ask feedback
> from someone familiar with drag-and-drop, also my patch already works in
> non-e10s mode, I cannot figure out how to pass a DOM node between the
> processes to finish the patch.
> 
> I'm hoping you can help me with your experiences in both drag-and-drop and
> e10s, thanks!

smaug or enn might be better for drag and drop questions, I know thew native widget bits but little about dome code.

For e10s, you can't pass a dom node between processes, you'll have to break it down into whatever static information you need and reconstitute it on the other side of the connection. What information are you trying to pass across?

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #27)
> 
> smaug or enn might be better for drag and drop questions, I know thew native
> widget bits but little about dome code.

Thanks for the suggestion!

> 
> For e10s, you can't pass a dom node between processes, you'll have to break
> it down into whatever static information you need and reconstitute it on the
> other side of the connection. What information are you trying to pass across?

The `requestingNode` for initializing the `nsDataObj::CStream` at [1], :ckerschb says "If you really can not query a requestingNode, then you would have to add some special do to the 'transferable' that accounts specificially for your case and pass the requestingPrincipal in that case" in comment 20.

[1]: https://dxr.mozilla.org/mozilla-central/rev/0a25833062a8/widget/windows/nsDataObj.cpp#76

    
    

    
  
Interesting, why did aRequestingNode assert there? What was it assigned as?

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #29)
> Interesting, why did aRequestingNode assert there? What was it assigned as?

In my patch, I'm trying to set the image element as the requestingNode of the nsITransferable (in `nsCopySupport::ImageCopy`). I think it doesn't work with e10s because the requestingNode part is lost in [1].

[1]: https://dxr.mozilla.org/mozilla-central/rev/0a25833062a8/widget/nsClipboardProxy.cpp#31

    
  

        Attachment #8748525 -
        Attachment is obsolete: true

    
    

    
  
Hector, just FYI I am away until late next week, so it will take me some time to get to your review.

    
    

    
  
(In reply to Seth Fowler [:seth] [:s2h] from comment #34)
> Hector, just FYI I am away until late next week, so it will take me some
> time to get to your review.

Hello Seth, thanks for letting me know. It's not like this bug is urgent. :-)

    
    

    
  
Comment on attachment 8767003 [details]
Bug 664717 - Part 1: Switch to requestingPrincipal in nsITransferable.

https://reviewboard.mozilla.org/r/61724/#review58550

Hey, I am fine with using the principal instead of the node within the transferable. I was not able to review nsDataObj.h though. I assume it's just the update within the arguments list switching from node to principal. If that's the case then go for it. r=me

::: widget/nsTransferable.cpp:639
(Diff revision 1)
>  
>  NS_IMETHODIMP
> -nsTransferable::GetRequestingNode(nsIDOMNode** outRequestingNode)
> +nsTransferable::GetRequestingPrincipal(nsIPrincipal** outRequestingPrincipal)
>  {
> -  nsCOMPtr<nsIDOMNode> node = do_QueryReferent(mRequestingNode);
> -  node.forget(outRequestingNode);
> +  nsCOMPtr<nsIPrincipal> principal = mRequestingPrincipal;
> +  principal.forget(outRequestingPrincipal);

NS_IF_ADDREF(*outRequestingPrincipal = mLoadingPrincipal);

        Attachment #8767003 -
        Flags: review?(ckerschb) → review+

    
    

    
  
Comment on attachment 8767003 [details]
Bug 664717 - Part 1: Switch to requestingPrincipal in nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61724/diff/1-2/

    
    

    
  
Comment on attachment 8767004 [details]
Bug 664717 - Part 2: Pass requestingPrincipal to parent process with nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61726/diff/1-2/

    
    

    
  
Comment on attachment 8767005 [details]
Bug 664717 - Part 3: Include file promise when copying an image in Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61728/diff/1-2/

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> 
> Hey, I am fine with using the principal instead of the node within the
> transferable. I was not able to review nsDataObj.h though. I assume it's
> just the update within the arguments list switching from node to principal.
> If that's the case then go for it. r=me

The diff of nsDataObj.h can be seen at https://reviewboard-hg.mozilla.org/gecko/diff/c4f15e1497aa/widget/windows/nsDataObj.h

> 
> ::: widget/nsTransferable.cpp:639
> (Diff revision 1)
> >  
> >  NS_IMETHODIMP
> > -nsTransferable::GetRequestingNode(nsIDOMNode** outRequestingNode)
> > +nsTransferable::GetRequestingPrincipal(nsIPrincipal** outRequestingPrincipal)
> >  {
> > -  nsCOMPtr<nsIDOMNode> node = do_QueryReferent(mRequestingNode);
> > -  node.forget(outRequestingNode);
> > +  nsCOMPtr<nsIPrincipal> principal = mRequestingPrincipal;
> > +  principal.forget(outRequestingPrincipal);
> 
> NS_IF_ADDREF(*outRequestingPrincipal = mLoadingPrincipal);

I think you mean `NS_IF_ADDREF(*outRequestingPrincipal = mRequestingPrincipal);` here.

    
  
Assignee: nobody → bzhao
Status: NEW → ASSIGNED

        Attachment #8767004 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8767004 [details]
Bug 664717 - Part 2: Pass requestingPrincipal to parent process with nsITransferable.

https://reviewboard.mozilla.org/r/61726/#review58634

    
    

    
  
Comment on attachment 8767005 [details]
Bug 664717 - Part 3: Include file promise when copying an image in Windows.

https://reviewboard.mozilla.org/r/61728/#review64688

Works for me. Keep in mind that I do not understand all the content stuff that is happening in this patch, so I'm just reviewing from the perspective of ImageLib.

::: dom/base/nsCopySupport.cpp:581
(Diff revision 2)
> +                                   nsIImageLoadingContent* aImageElement)
> +{
> +  nsresult rv;
> +
> +  NS_ENSURE_TRUE(aImgRequest, NS_OK);
> +  nsCOMPtr<nsINode> node(do_QueryInterface(aImageElement, &rv));

Nit: it's Mozilla style to use assignment for initialization where possible. In this case, this would be better written as:

nsCOMPtr<nsINode> node = do_QueryInterface(aImageElement, &rv);

It's not a big deal, though.

::: dom/base/nsCopySupport.cpp:593
(Diff revision 2)
> +
> +  nsCOMPtr<nsIURI> imgUri;
> +  rv = aImgRequest->GetCurrentURI(getter_AddRefs(imgUri));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri));

Same nit applies here.

        Attachment #8767005 -
        Flags: review?(seth.bugzilla) → review+

    
    

    
  
Comment on attachment 8767003 [details]
Bug 664717 - Part 1: Switch to requestingPrincipal in nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61724/diff/2-3/

    
    

    
  
Comment on attachment 8767004 [details]
Bug 664717 - Part 2: Pass requestingPrincipal to parent process with nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61726/diff/2-3/

    
    

    
  
Comment on attachment 8767005 [details]
Bug 664717 - Part 3: Include file promise when copying an image in Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61728/diff/2-3/

    
    

    
  
Comment on attachment 8767003 [details]
Bug 664717 - Part 1: Switch to requestingPrincipal in nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61724/diff/3-4/

    
    

    
  
Comment on attachment 8767004 [details]
Bug 664717 - Part 2: Pass requestingPrincipal to parent process with nsITransferable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61726/diff/3-4/

    
    

    
  
Comment on attachment 8767005 [details]
Bug 664717 - Part 3: Include file promise when copying an image in Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61728/diff/3-4/

    
    

    
  
(In reply to Hector Zhao [:hectorz] from comment #49)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7980ed853b7db043aa4094be9760ddb69bb6d7dc

This was the patches rebased onto central from earlier today.

During an earlier try run triggered from MozReview (https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6943c95ee59d82b1ffc3fdfc867bd8344e61cd), there're multiple fails/retries of Mochitest-1 on Windows 7 VM debug. I'm assuming they're all somewhat intermittent.

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/085a9b02aa2b
Part 1: Switch to requestingPrincipal in nsITransferable. r=ckerschb
https://hg.mozilla.org/integration/fx-team/rev/466c454e0393
Part 2: Pass requestingPrincipal to parent process with nsITransferable. r=smaug
https://hg.mozilla.org/integration/fx-team/rev/457ebb2b73ad
Part 3: Include file promise when copying an image in Windows. r=seth
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/085a9b02aa2b
https://hg.mozilla.org/mozilla-central/rev/466c454e0393
https://hg.mozilla.org/mozilla-central/rev/457ebb2b73ad
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

    
    

    
  
Verified on Firefox 51.
Status: RESOLVED → VERIFIED

    
  
Depends on: 1307999

    
  
Depends on: 1318736

    
    

    
  
Updating status on 51 per Comment 53.

          status-firefox51:
          fixed → verified
Depends on: 1342740

    
  
Depends on: 1340039

    
  
Depends on: 1368464

    
    

    
  
I'm on FF 96, Mac OS and this is not fixed.


Copying a gif with "copy image" will copy a one-frame PNG of the gif, instead of the gif itself.



    
    

    
  
[Tracking Requested - why for this release]:



          status-firefox96:
          --- → affected

          tracking-firefox96:
          --- → ?

    
    

    
  
(In reply to L from comment #58)




[Tracking Requested - why for this release]:




Please File a new bug for this.


No longer depends on: 1368464
Regressions: 1368464

    
  
See Also:  → 1854747

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: