638523 - Save Image saves htm, not the image

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Thomas Arend [:tarend]]

(1) go to m.spiegel.de or other page with images
(2) tap and hold an image
(3) select "save image"

Result: Fennec downloads htm file instead of the image
Expected: Download image or remove option "save image" for non-images

Comment 1

[Matt Brubeck (:mbrubeck)]

I can't reproduce this on desktop or Android.  Can you test again?

If an image is also a link, then "Save Link" and "Save Image" appear right next to each other in the menu.  One thing that might have happened is that the touch might have registered on the "Save Link" button instead.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I can reproduce it on desktop by using phony to emulate android. :/

Comment 3

[Matt Brubeck (:mbrubeck)]

Okay, I can reproduce it too, but only on certain sites.  m.spiegel.de has this bug, but only if you have the right useragent - otherwise it redirects you to their non-mobile site which does not have the bug.

It seems to happen when the image URI does not include a file extension.  For example, I see it on Daring Fireball which has:

<img src="/graphics/logos/" alt="Daring Fireball" height="56" />

For some reason, Fennec adds ".htm" to the filename when there is no extension, and then thinks that the file has type text/html even though it is actually an image.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

<img src="http://m.spiegel.de/img/ic?width=304&amp;height=480&amp;fsize=999000&amp;format=jpg&amp;url=http%3A%2F%2Fimages.spiegel.netbiscuits.com%2F%2Fimages%2Fspon%2Fimage-187635-panoV9-wqwp.jpg">

I suppose our parsing code messed up the name...

Comment 5

[Matt Brubeck (:mbrubeck)]

The query string (everything after the "?") is usually not treated as part of the filename, otherwise a URI like http://example.com/image.png?width=100&height=100 would not get the right name.

Doing some debugging, I found that imageCache.findEntryProperties is returning null here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#149

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I've test this patch under m.spiegel.de and daring fireball and it works nice.

I don' have a complete answer about why the checks for the cache entry fails, it could be because some cache entries are hidden to the chrome UI as a consumer.... (we fail here modules/libpr0n/src/imgLoader.cpp#957) as said by http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsICacheService.idl#56

I suggest taking this patch for the release (it is low risk) and remove it later if someone has the response and a better way to do that.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/cffb1281d2bd

Comment 8

[Thomas Arend [:tarend]]

Has this patch landed, yet? In the current nightly 20110305 'Save Image' does not work for me at all!

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to comment #8)
> Has this patch landed, yet? In the current nightly 20110305 'Save Image' does
> not work for me at all!

I'm seeing the same thing, on Android and desktop.  When I try to save an image I get:

spec=/logos
WARNING: malformed url: no scheme: file /home/mbrubeck/src/mozilla/mozilla-central/netwerk/base/src/nsStandardURL.cpp, line 764

From hg bisect, it looks like this might be related to the DEFER_OPEN patch from bug 634666:
http://hg.mozilla.org/mobile-browser/rev/a59a3d731222

...although I was not updating mozilla-central at the same time, so that might just be a different issue caused by my repos getting out of sync.

I also noticed that Fennec started popping up a file save dialog on desktop, after this change landed but before a59a3d731222.

Comment 10

[Matt Brubeck (:mbrubeck)]

On Android, in the JS console I get NS_ERROR_NOT_IMPLEMENTED [nsIFilePicker.init] from contentAreaUtils.js :: getTargetFile :: line 561.

I think this is probably a regression from this bug.  Reopening.

Comment 11

[Matt Brubeck (:mbrubeck)]

The patch from this bug changed skipPrompt from true to false.  Vivien mentioned he was doing this for debugging purposes; it looks like he forgot to change it back, and I missed this during review.  Pushed a breakage fix to set skipPrompt = true again: http://hg.mozilla.org/mobile-browser/rev/62c013ee422c

This fixes the regression.  This bug is now fixed on desktop, but only partially fixed on Android.

Before these patches, <img src="logos/"> was saved as "logos.htm."  Now on desktop it is saved correctly as "logos.png" but on Android it is saved as just "logos" and cannot be opened.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #11)
> The patch from this bug changed skipPrompt from true to false.  Vivien
> mentioned he was doing this for debugging purposes; it looks like he forgot to
> change it back, and I missed this during review.  Pushed a breakage fix to set
> skipPrompt = true again: http://hg.mozilla.org/mobile-browser/rev/62c013ee422c
> 
> This fixes the regression.  This bug is now fixed on desktop, but only
> partially fixed on Android.
> 
> Before these patches, <img src="logos/"> was saved as "logos.htm."  Now on
> desktop it is saved correctly as "logos.png" but on Android it is saved as just
> "logos" and cannot be opened.

Sorry for the leftover and thanks for the breakage fix. I will look into why Android does not add the .png extension in this case.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Brad told me it will fix bug 632850 and this one in the same row.
Problem on android is: there is a failure to map the mimetype to an extension

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

there is no dependency on bug 632850

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 16

[Doug Turner (:dougt)]

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

please rename getMimeTypeFromExtensions().  it no longer accepts multiple extensions.

import android.webkit.MimeTypeMap.  That will simplify getExtensionFromMimeType.

what is the string conversion here for:

	

  bridge->GetExtensionFromMimeType(nsCAutoString(aMimeType), fileExt);


Isn't there a AssignAscii or something?

    aFileExt.Assign(NS_ConvertUTF16toUTF8(jniStr.get()));

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 18

[Doug Turner (:dougt)]

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

so, GeckoAppShell.getExtensionFromMimeType can return null.  Not sure what you want to do, but you may end up with a file named "foopy."

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #18)
> Comment on attachment 517648 [details] [diff] [review]
> patch
> 
> so, GeckoAppShell.getExtensionFromMimeType can return null.  Not sure what you
> want to do, but you may end up with a file named "foopy."

yea, it could happen

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #18)
> Comment on attachment 517648 [details] [diff] [review]
> patch
> 
> so, GeckoAppShell.getExtensionFromMimeType can return null.  Not sure what you
> want to do, but you may end up with a file named "foopy."

On android mimetype to extension mapping and exension to mimetype mapping is the same map, so if we can't produce an extension from a mimetype, we won't be able to recreate that mimetype from any extension an therefore won't be able to get a helper app to open the file.

So in the end, anything we do to bandaid the possibility will eventually blow up, so there's no reason to mask it. And I don't know what a good bandaid would be anyway.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/e28388957c4d

Comment 22

[Anna (Waverley)]

VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110316 Firefox/4.0b13pre Fennec /4.0b6pre 

Device: Motorola Droid 2 (Android 2.2)