Closed Bug 638523 Opened 9 years ago Closed 9 years ago

Save Image saves htm, not the image

Categories

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

ARM
Android
defect

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: tarend, Assigned: blassey)

References

Details

Attachments

(2 files, 1 obsolete file)

(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
Assignee: nobody → 21
tracking-fennec: ? → 2.0+
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
I can reproduce it on desktop by using phony to emulate android. :/
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
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
Attached patch PatchSplinter Review
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.
Attachment #516786 - Flags: review?(mbrubeck)
Attachment #516786 - Flags: review?(mbrubeck) → review+
Whiteboard: [has patch][can land]
http://hg.mozilla.org/mobile-browser/rev/cffb1281d2bd
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Has this patch landed, yet? In the current nightly 20110305 'Save Image' does not work for me at all!
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has patch][can land] → [needs new patch]
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.
(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.
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
Assignee: 21 → blassey.bugs
Depends on: 632850
there is no dependency on bug 632850
No longer depends on: 632850
Attached patch patch (obsolete) — Splinter Review
Attachment #517517 - Flags: review?(doug.turner)
Whiteboard: [needs new patch] → [needs-review (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()));
Attachment #517517 - Flags: review?(doug.turner) → review-
Attached patch patchSplinter Review
Attachment #517517 - Attachment is obsolete: true
Attachment #517648 - Flags: review?(doug.turner)
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."
(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
(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.
Attachment #517648 - Flags: review?(doug.turner) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/e28388957c4d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs-review (dougt)]
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)
Status: RESOLVED → VERIFIED
Blocks: 777267
You need to log in before you can comment on or make changes to this bug.