1054276 - In the "media" view, the "save as" button saves images with the wrong extension

Status: RESOLVED DUPLICATE of bug 415225

(Firefox :: Page Info Window, defect)

Description

[Ophir LOJKINE]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715214327

Steps to reproduce:

* Go to http://ophir.lojkine.free.fr/FF-bug/bug.html
* Type Ctrl+I (or go to Tools > Page information)
* Go to the "Media" tab
* Click "Save As"


Actual results:

Firefox proposes to save the file as "png_image_without_extension.htm"


Expected results:

It should have proposed "png_image_without_extension.png" (or at least just "png_image_without_extension")

Comment 1

[Loic]

Probably because your PNG is hosted without the .png extension.

Comment 2

[Ophir LOJKINE]

(In reply to Loic from comment #1)
> Probably because your PNG is hosted without the .png extension.

Yes, that's the bug. The file has no extension, and firefox adds an "htm" extension.
The bug doesn't occur when the files are stored locally, and achessed via the "file://" protocol.

Comment 3

[:Gijs (he/him)]

This is an issue with the page info window specifically - the context menu works fine in this case. This is because the saveURL function that the media tab uses has no mimetype info (and doesn't bother getting any), while the context menu's saveImageURL here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#89

does quite some work to get this information for images. For media stuff, the context menu does even more work:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1121

We should probably factor that out and use it from the media tab. CC'ing Florian who might know if there's already a bug on this...

Comment 4

[Florian Quèze [:florian]]

Bug 733299 seems related (the summary there describes the proposed solution instead of the problem).

Comment 5

[Matt Hammerly [:mathu]]

Attachment: browser_bug1054276_pageInfoContentType.diff

This patch fixes the bug where extensionless images would have a .htm image appended in the pageinfo window. This fix only solves the problem in non-e10s windows.

Audio and video in non-e10s have the same problem as images. This fix doesn't address that. I'll look into translating the content type detection code from the context menu to something usable here, but I don't know if that should get its own bug so I'm submitting this for now. 

The problem with e10s is another issue. This patch changes a call from saveURL() to saveImageURL() and the latter attempts to send a CPOW to a native C++ function: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#114
Investigating that is on my todo list after the above task, unless someone more experienced with e10s refactoring wants to take that.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Mike, can you help Matt here with his CPOW question (or redirect to someone else if you're not able to)?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8572421 [details] [diff] [review]
browser_bug1054276_pageInfoContentType.diff

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

::: browser/base/content/pageinfo/pageInfo.js
@@ +795,5 @@
>          titleKey = "SaveVideoTitle";
>        else if (item instanceof HTMLAudioElement)
>          titleKey = "SaveAudioTitle";
>  
> +      saveImageURL(url, null, titleKey, false, false, makeURI(item.baseURI), gDocument);

We should only be calling saveImageURL if the item is not an instanceof HTMLVideoElement or HTMLAudioElement.

Comment 8

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

(In reply to Matt Hammerly [:mathu] from comment #5)
> Created attachment 8572421 [details] [diff] [review]
> browser_bug1054276_pageInfoContentType.diff
> 
> This patch fixes the bug where extensionless images would have a .htm image
> appended in the pageinfo window. This fix only solves the problem in
> non-e10s windows.
> 
> Audio and video in non-e10s have the same problem as images. This fix
> doesn't address that. I'll look into translating the content type detection
> code from the context menu to something usable here, but I don't know if
> that should get its own bug so I'm submitting this for now. 
> 
> The problem with e10s is another issue. This patch changes a call from
> saveURL() to saveImageURL() and the latter attempts to send a CPOW to a
> native C++ function:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/
> contentAreaUtils.js#114
> Investigating that is on my todo list after the above task, unless someone
> more experienced with e10s refactoring wants to take that.

Hm.

The Page Info dialog is kind of a sad story, e10s-wise. It does a ton of DOM walking through unsafe CPOWs in order to populate itself, and all of its manipulation occurs through unsafe CPOWs as well.

The work to fix up the Page Info dialog to not use unsafe CPOWs is being tracked in bug 1040947.

Perhaps in the e10s case (which you can detect by checking to see if the document is a CPOW via Components.utils.isCrossProcessWrapper(doc)), you might have saveImageURL bypass any caching and pass null as the document to internalSave? That might keep the functionality working for the e10s case, while fixing this bug for the normal case.

If we were to do that, I'd want us to file a follow-up bug to fix retrieving from the cache in the e10s case.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Any update on this work, Matt?

Comment 10

[Matt Hammerly [:mathu]]

Sorry for the radio silence. I got called into work Saturday for server upgrades and then helped a friend move on Sunday.

When Jared and I spoke on the phone the other day, he said that when he used the Page Info window's Save As button in non-e10s on audio and video, Firefox did NOT append a file extension if the file in question didn't have one. I cannot recreate that behavior; on my end, ".htm" is appended to any media that doesn't come with a file extension. The context menu does not append a ".htm".

My test cases are here:
PNG: http://ophir.lojkine.free.fr/FF-bug/bug.html
MP3: http://mathu.tk/audio.htm (Like Tetanus In A Wound by Hussalonia -- public domain mp3 swiped from https://archive.org/details/The_Public_Domain_EP )
WEBM: http://mathu.tk/video.htm (swiped from imgur http://imgur.com/t/gifv/hCUZaLf )

In the page info window's Save As button, all three have ".htm" appended. In the context menu's Save As option, the PNG gets ".png" appended and the audio and video have nothing appended.

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js#812
Detecting whether the target is an audio or video element works properly. The extension is still guessed wrong. Unambiguous screenshots: http://mathu.tk/screens/audio.png http://mathu.tk/screens/video.png
Is it worth digging into the context menu's function to see how it works? https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1325

The CPOW thing doesn't actually change much. See here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#122
If cache is not being bypassed, there is an exception thrown here in both the e10s and non-e10s cases. In the e10s case, its message reads "It's illegal to pass a CPOW to native code arg 0 [imgITools.getImgCacheForDocument]" and neither the content type nor content disposition are read. In the non-e10s case, the exception's message reads "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]" and it is thrown after getting the content type and while getting the content disposition.

I will submit a patch doing what Mike suggests shortly. Taking care of the e10s case as he describes doesn't actually change anything about how the functionality works, but it is one fewer exception.

Comment 11

[Matt Hammerly [:mathu]]

Attachment: browser_bug1054276_pageInfoContentType.diff

Fixes the bug in the non-e10s case for images only. Audio and video are subject to the same bug. Disables searching the cache in the e10s case to avoid an exception that doesn't ultimately change anything about how the function works. Would require a follow-up bug to re-enable saving the image from cache in the e10s case.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matt Hammerly [:mathu] from comment #10)

This gets the debugger to step through the Page Info code for me:
Open http://mathu.tk/video.htm
Open the Page Info window
Open the Browser Toolbox
In the debugger, open pageInfo.js
Place a breakpoint at the call to saveURL

Now when I click on Save As... the breakpoint gets hit. I actually don't need to place a `debugger;` statement to get it to work.

Where the bug happens for video/audio, http://screencast.com/t/SQRQCf83gII

This is happening because internalSave is not being passed the contentType at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#89. With no contentType, it uses .htm as the extension.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8575083 [details] [diff] [review]
browser_bug1054276_pageInfoContentType.diff

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

This looks good to me. Does my most recent comment help you with the audio/video? We could handle that in a separate bug if you find it will be much more work.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Any update here?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Closing old needinfo request...

Comment 17

[alexmehler]

ok guys , since this seems a super long standing bug .. 


Ff you use skip prompt on the saveImage filepicker the same bug happens and the page info save function has it on MANY websites! 

BUT

pageinfo.js already has a method of determining the mimetype almost perfectly , by going through the cache.

i isolated these functions:
 
//////////////////////////
const nsICacheStorageService = Components.interfaces.nsICacheStorageService;
const nsICacheStorage = Components.interfaces.nsICacheStorage;
const cacheService = Components.classes["@mozilla.org/netwerk/cache-storage-service;1"].getService(nsICacheStorageService);

Components.utils.import("resource://gre/modules/LoadContextInfo.jsm");

var loadContextInfo = LoadContextInfo.fromLoadContext(
  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
        .getInterface(Components.interfaces.nsIWebNavigation)
        .QueryInterface(Components.interfaces.nsILoadContext), false);
var diskStorage = cacheService.diskCacheStorage(loadContextInfo, false);

function openCacheEntry(key, cb) {
  var checkCacheListener = {
    onCacheEntryCheck(entry, appCache) {
      return Components.interfaces.nsICacheEntryOpenCallback.ENTRY_WANTED;
	  },
    onCacheEntryAvailable(entry, isNew, appCache, status) {
      cb(entry);
    }
  };
  diskStorage.asyncOpenURI(Services.io.newURI(key), "", nsICacheStorage.OPEN_READONLY, checkCacheListener);
}

function getContentTypeFromHeaders(cacheEntryDescriptor) {
  if (!cacheEntryDescriptor)
    return null;

  let headers = cacheEntryDescriptor.getMetaDataElement("response-head");
  let type = /^Content-Type:\s*(.*?)\s*(?:\;|$)/mi.exec(headers);
  return type && type[1];
}
//////////////////////




copy those to your code

then you can easily detect the mimetype with:



//////////////////
let imageUrl = "aImageUrl";  //  this image is already loaded and should be in the cache 

  var cacheKey = imageUrl.replace(/#.*$/, "");
  openCacheEntry(cacheKey, function(cacheEntry) {
    
    if (cacheEntry){

      var mimeType =  getContentTypeFromHeaders(cacheEntry);
      console.log(mimeType);  // image/jpeg or whatever
    }
  }); 
/////////////////////////

this method has an almost 100% detection rate (there are obviously cases which wont work) , works also on google image search and other php generated image urls ... 

its instant and uses minimal cpu

Comment 18

[alexmehler]

ok just additionally :

theres also 

image/public/imgICache.idl
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/imgICache


that method might be even more reliable

Comment 19

[alexmehler]

based on comment 17 i wrote this little ugly ;)

var mimetype = null ;
if(defaultFileName.indexOf(".") < 0 ){   // from getDefaultFileName() 

	if (imageUrl.startsWith("data:image/") ){
		mimetype=imageUrl.replace(/data:/g, "").replace(/;.*/g, "");  //for bas64 images 
	}
	if(mimetype == null ){
		var cacheKey = imageUrl.replace(/#.*$/, "");
		openCacheEntry(cacheKey, function(cacheEntry) {
			if (cacheEntry) {
				mimetype = getContentTypeFromHeaders(cacheEntry);   //from cache entry
				internalSave(imageUrl, null, null, null,mimetype, false, "SaveImageTitle",null, null, null
                                ,null,null, null);
			}else{
				internalSave(imageUrl, null, null, null,null, false, "SaveImageTitle",null, null, null,
                                null,null, null);
			} 
		}); 
	}else{
		internalSave(imageUrl, null, null, null,mimetype, false, "SaveImageTitle",null, null, null, (! prompt),null, 
                null);
	}
}else{
	internalSave(imageUrl, null, null, null,null, false, "SaveImageTitle",null, null, null, null,null, null);}




this detects filename on all strange cases i could check ...  just add a nicer version of it to the contentareautils.js or whereever it should go.

in the page info dialog pageInfo.js, this obviously happend automatically and the mimetype is already saved in the saved gImageElement object

Comment 20

[alexmehler]

openCacheEntry is async,  getContentTypeFromHeaders is sync

Comment 21

[Djfe]

This is a duplicate of Bug 415225
and was just solved, jay :)

Comment 22

[:Gijs (he/him)]

Thanks for flagging!