1122525 - [e10s] Save image as wrong type

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Alexandre Vicenzi]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150115030228

Steps to reproduce:

Access this url: http://boards.dingoonity.org/index.php?action=dlattach;attach=3913;type=avatar

1. Try save the image.
2. Try copy the image and paste to Paint.



Actual results:

1. Save as is trying to save as "index.php".
2. Image is not copied to clipboard.


Expected results:

1. The same as non-e10s. Save as ".PNG".
2. Copy to clipboard as non-e10s.

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed in 38.0a1 (2015-01-16), Win 7

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Works for me on Mac 10.10, in case that helps you narrow anything down.

Comment 3

[Jim Mathies [:jimm]]

Hey guys,

Just tested this in nightly/win7. I can reproduce the copy paste bug (that's filed in an m5 bug). But i can't reproduce the save image as bug, when i save the avatar it saves out as a ping. Can you all confirm again?

Comment 4

[Paul Silaghi, QA [:pauly]]

Confirmed saving as .png is working fine again starting with 2015-01-18 build.

(In reply to Jim Mathies [:jimm] from comment #3)
> the copy paste bug (that's filed in an m5 bug)
feel free to dupe to that

Comment 6

[Paul Silaghi, QA [:pauly]]

Reopening this based on following STR:
1. open google.com
2. search for "bmw"
3. click on "images" tab
4. right click/save image as on any image

AR:
no extension for saved images

Comment 7

[Jim Mathies [:jimm]]

confirming - reproducible on win.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The image at http://ophir.lojkine.free.fr/FF-bug/bug.html tries to save with a .htm extension.

Comment 9

[Jim Mathies [:jimm]]

The problem is right here -

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#108

aDoc is a cpow so getImgCacheForDocument(aDoc) fails. Normally contentType would be image/png but in this case it's null.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Accumulate the mime info from the cache in the child, and forward that over.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8576526 [details] [diff] [review]
patch

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

Hey jimm! This looks good - just a few minor points. Let me know if you have any questions about them.

::: browser/base/content/content.js
@@ +169,5 @@
>        spellInfo =
>          InlineSpellCheckerContent.initContextMenu(event, editFlags, this);
>      }
>  
> +    // Media related cache info parent needs for saving

This entire addition should probably get moved out of this if block so that the non-e10s case can take advantage of the information as well.

@@ +176,5 @@
> +    if (event.target.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
> +        event.target instanceof Ci.nsIImageLoadingContent &&
> +        event.target.currentURI) {
> +      try {
> +        var imageCache = Components.classes["@mozilla.org/image/tools;1"]

let, not var

Also, you have the Cc and Ci shortcuts available in this scope, so you can replace Components.classes/interfaces.

@@ +179,5 @@
> +      try {
> +        var imageCache = Components.classes["@mozilla.org/image/tools;1"]
> +                                    .getService(Components.interfaces.imgITools)
> +                                    .getImgCacheForDocument(doc);
> +        var props =

let, not var

@@ +182,5 @@
> +                                    .getImgCacheForDocument(doc);
> +        var props =
> +          imageCache.findEntryProperties(event.target.currentURI);
> +        if (props) {
> +          contentType = String(props.get("type", Ci.nsISupportsCString));

I think you can just use:

props.get("foo", Ci.nsISupportsCString).data;

To get JS string. I think I'd prefer that over wrapping in a String().

@@ +195,5 @@
>      let principal = doc.nodePrincipal;
>      sendSyncMessage("contextmenu",
>                      { editFlags, spellInfo, customMenuItems, addonInfo,
> +                      principal, docLocation, charSet, baseURI, referrer,
> +                      contentType, contentDisposition },

You need to add contentType and contentDisposition to gContextMenuContentData in the non-remote case (see line 206 down).

::: browser/base/content/nsContextMenu.js
@@ +1311,5 @@
>      var doc =  this.target.ownerDocument;
>      if (this.onCanvas) {
>        // Bypass cache, since it's a data: URL.
>        saveImageURL(this.target.toDataURL(), "canvas.png", "SaveImageTitle",
> +                   true, false, BrowserUtils.makeURIFromCPOW(doc.documentURIObject),

While you're here, gContextMenuContentData.documentURIObject instead of BrowserUtils.makeURIFromCPOW(doc.documentURIObject) will let you avoid an unsafe CPOW.

@@ +1317,4 @@
>      }
>      else if (this.onImage) {
>        urlSecurityCheck(this.mediaURL, this.principal);
> +      let uri = BrowserUtils.makeURIFromCPOW(doc.documentURIObject);

Can you use gContextMenuContentData.documentURIObject here instead, too?

::: toolkit/content/contentAreaUtils.js
@@ +104,5 @@
>    forbidCPOW(aURL, "saveImageURL", "aURL");
>    forbidCPOW(aReferrer, "saveImageURL", "aReferrer");
>    // Allow aSourceDocument to be a CPOW.
>  
> +  var contentType = aContentType;

let, not var - though, why are you re-assigning these? Why not just use aContentType and aContentDisp?

Comment 12

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8586076 [details] [diff] [review]
patch v.2

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

::: browser/base/content/content.js
@@ +179,5 @@
> +      if (props) {
> +        contentType = props.get("type", Ci.nsISupportsCString).data;
> +        contentDisposition = props.get("content-disposition", Ci.nsISupportsCString).data;
> +        if (contentType) {
> +          contentType = contentType.toString();

Not sure if I still need these toString routines. I think I do after making the change you suggested, since we're still dealing with this weird object that wraps a c string. Unfortunately the content toolbox is busted at the moment, so I wasn't able to check for sure.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8586076 [details] [diff] [review]
patch v.2

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

r=me with my final nits addressed. Thanks!

::: browser/base/content/content.js
@@ +179,5 @@
> +      if (props) {
> +        contentType = props.get("type", Ci.nsISupportsCString).data;
> +        contentDisposition = props.get("content-disposition", Ci.nsISupportsCString).data;
> +        if (contentType) {
> +          contentType = contentType.toString();

Hrm. I'm uncertain that we need that. Once we get the data from the nsISupportsCString, I'm pretty sure we're just a straight JS string at that point. I don't believe there's any need to convert it.

@@ +185,5 @@
> +        if (contentDisposition) {
> +          contentDisposition = contentDisposition.toString();
> +        }
> +      }
> +    } catch (e) {

Worth doing Cu.reportError with the exception?

@@ +222,5 @@
>        charSet: charSet,
>        referrer: referrer,
>        referrerPolicy: referrerPolicy,
> +      contentType: contentType,
> +      contentDisposition: contentDisposition

Trailing comma please

::: browser/base/content/tabbrowser.xml
@@ +3719,5 @@
>  
>        <method name="receiveMessage">
>          <parameter name="aMessage"/>
> +        <body>
> +                                            <![CDATA[

Busted indentation

Comment 15

[Jim Mathies [:jimm]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> Comment on attachment 8586076 [details] [diff] [review]
> patch v.2
> 
> Review of attachment 8586076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with my final nits addressed. Thanks!
> 
> ::: browser/base/content/content.js
> @@ +179,5 @@
> > +      if (props) {
> > +        contentType = props.get("type", Ci.nsISupportsCString).data;
> > +        contentDisposition = props.get("content-disposition", Ci.nsISupportsCString).data;
> > +        if (contentType) {
> > +          contentType = contentType.toString();
> 
> Hrm. I'm uncertain that we need that. Once we get the data from the
> nsISupportsCString, I'm pretty sure we're just a straight JS string at that
> point. I don't believe there's any need to convert it.

Ok, took these out, will test manually.

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch r=mconley

Comment 17

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e372f13accff

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/acd77f938db4

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acd77f938db4

Comment 20

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced the bug in 38.0a1 (2015-01-16) on Linux x64 by following Comment 0's instruction!

This bug fix is verified on Latest beta 40.0b3 (Build ID: 20150709163524) and
Latest Nightly 42.0a1 (2015-07-10) (Build ID: 20150710030206)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 21

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 38.0a1(2015-01-16)(Build: 20150116030203)on 
windows 8.1 pro x64 with the instructions from comment 0 .

Verified as fixed with Latest Firefox beta 40.0b3 (Build ID: 20150709163524)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0

Verified as fixed with Latest Firefox Nightly 42.0a1 (Build ID: 20150711030210)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 22

[Nazir Ahmed Sabbir [:NaSb]]

As per Comment 20 and Comment 21, The bug is fixed now.

Comment 23

[yamishi.atarashianime]

works on debian 9 (stretch) (testing)