Closed
Bug 1122525
Opened 9 years ago
Closed 9 years ago
[e10s] Save image as wrong type
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(e10sm6+, firefox40 fixed, firefox42 verified)
VERIFIED
FIXED
mozilla40
People
(Reporter: vicenzi.alexandre, Assigned: jimm)
References
Details
(Whiteboard: [testday-20150710])
Attachments
(1 file, 2 obsolete files)
8.22 KB,
patch
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Confirmed in 38.0a1 (2015-01-16), Win 7
Blocks: e10s
Status: UNCONFIRMED → NEW
Component: Untriaged → File Handling
Ever confirmed: true
Product: Firefox → Core
Comment 2•9 years ago
|
||
Works for me on Mac 10.10, in case that helps you narrow anything down.
Assignee | ||
Comment 3•9 years ago
|
||
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?
Flags: needinfo?(paul.silaghi)
Comment 4•9 years ago
|
||
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
Flags: needinfo?(paul.silaghi)
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Comment 6•9 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 7•9 years ago
|
||
confirming - reproducible on win.
Updated•9 years ago
|
Assignee: nobody → jmathies
tracking-e10s:
--- → m6+
The image at http://ophir.lojkine.free.fr/FF-bug/bug.html tries to save with a .htm extension.
Assignee | ||
Updated•9 years ago
|
Version: 37 Branch → Trunk
Assignee | ||
Comment 9•9 years ago
|
||
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.
Assignee | ||
Comment 10•9 years ago
|
||
Accumulate the mime info from the cache in the child, and forward that over.
Attachment #8576526 -
Flags: review?(mconley)
Comment 11•9 years ago
|
||
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?
Attachment #8576526 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8576526 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8586076 -
Flags: review?(mconley)
Assignee | ||
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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
Attachment #8586076 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8586076 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e372f13accff
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/acd77f938db4
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/acd77f938db4
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
Updated•9 years ago
|
QA Whiteboard: [good first verify][verify in Nightly only]
Comment 20•9 years ago
|
||
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
QA Whiteboard: [good first verify][verify in Nightly only] → [good first verify][verify in Nightly only][testday-20150710]
Whiteboard: [testday-20150710]
Comment 21•9 years ago
|
||
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•9 years ago
|
||
As per Comment 20 and Comment 21, The bug is fixed now.
Status: RESOLVED → VERIFIED
status-firefox42:
--- → verified
Comment 23•9 years ago
|
||
works on debian 9 (stretch) (testing)
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•