741432 - imgLoader::FindEntryProperties should throw on null URI instead of crashing ("save image" in Fennec passes null URIs)

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect)

Description

[Jason Smith [:jsmith]]

Steps:

	1. Install the latest fennec native nightly
	2. Go to apps.mozillalabs.com/appdir
	3. Select an app to install and hit ok
	4. Go to about:apps
	5. Long press the app you installed
	6. Select Save Image

Expected:

The icon of the app should be saved to the user's machine. Although in general, I do not understand why this feature is even part of web apps (it does not seem relevant).

Actual:

Nightly crashes. See https://crash-stats.mozilla.com/report/index/bp-f1e781aa-bbae-4956-82c9-ca1cd2120401 for details. Failure appears to be occurring on imgLoader::GetCache. Might be related to bug 503237.

Device: Samsung Galaxy Nexus
OS: Android 4.02
Build: Fennec Native Nightly

Comment 1

[Aaron Train [:aaronmt]]

Is this consistently reproducible? (I think "Save Image" might be unintentionally available)

Comment 2

[Jason Smith [:jsmith]]

Yes.

Comment 3

[Scoobidiver (away)]

I think it should be classified in the Core-ImageLib component.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The image loader is crashing in this case because someone passed in a null nsIURI* to imgLoader::FindEntryProperties.

While a null-check in the image loader might not be a bad idea, that won't exactly make "save image" work in your case; it'll just throw an exception instead.

Comment 5

[Jason Smith [:jsmith]]

Sounds like there's multiple components to this bug then. From the core side, maybe an assertion might be good to throw in for the null check? The caller is coming from fennec native, so would the fix be on their side as well?

Comment 6

[Boris Zbarsky [:bzbarsky]]

If fennec native works a working "save image" in this case, they certainly need to fix _something_.

Comment 7

[Wesley Johnston (:wesj)]

We don't want save image in this case. Our plan to "fix" it is to implement html5 context menu support so that we do not show this item.

Until thats done (next release), we'll have to put in a hack to hide it, or just live with a little strangeness.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This bug is not restricted to web apps.

Fennec Native trunk 2012-04-29, Android 4.0.4 (stock), Google Nexus S

Choosing "Save Image" from the context menu crashes me with the following steps to reproduce:

1. Visit http://www.computerbase.de/forum/forumdisplay.php?f=51
2. After some time (>10 minutes?), visit it again. Now some threads will have square icons in front of the thread title, indicating new posts have been made.
3. Tap this icon and hold until the context menu is shown.
4. Select "Save Image".

Actual result: Fennec native crashes

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: logcat of crash

An example for a crash report is https://crash-stats.mozilla.com/report/index/60ba97f7-cef1-4685-954d-0395b2120430.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Just a note: "Save Image" is no longer shown for web apps. I am morphing this bug to be about general images.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This really really needs a separate bug on the Fennec UI to stop passing in null.  Could someone who understands where such bugs go please file that?

Comment 12

[Boris Zbarsky [:bzbarsky]]

And in particular, if this is an imagelib bug, then resummarizing accordingly.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> This really really needs a separate bug on the Fennec UI to stop passing in
> null.  Could someone who understands where such bugs go please file that?

Filed bug 750243

Comment 14

[Scoobidiver (away)]

It's #10 top crasher in 14.0a2 over the last 3 days.

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Naoki to retest this bug.

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I wonder if this is an ICS bug?  I tried to repro based on comment 8, but I was not able to repro on a Gingerbread / Samsung Galaxy S II.  Unfortunately, a dev is borrowing my ICS phone ATM to fix some ICS bugs.

Archaeopteryx, can you please try again to reproduce the bug?

Comment 17

[Jason Smith [:jsmith]]

(In reply to Naoki Hirata :nhirata from comment #16)
> I wonder if this is an ICS bug?  I tried to repro based on comment 8, but I
> was not able to repro on a Gingerbread / Samsung Galaxy S II. 
> Unfortunately, a dev is borrowing my ICS phone ATM to fix some ICS bugs.
> 
> Archaeopteryx, can you please try again to reproduce the bug?

Naoki - If you are in the Mt View office today, you could borrow my phone quickly to try to reproduce this bug if you wish.

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Unfortunately, I am not in the Mt View office today.  I am working from home.

I believe that it shouldn't be a ICS only bug.   Can you please retest to confirm, Archaeopteryx?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Just a note. With bug 750243 fixed, it should be much more difficult to see this bug anymore.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

I'm unable to reproduce with 2012-05-03 (tried 4 times, the first time the application died while I tried to trigger the context menu, the other 3 times I was able to save the image).

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> Just a note. With bug 750243 fixed, it should be much more difficult to see
> this bug anymore.

Should we leave this open still?  Or should we close it off as WFM as we don't seem to crash in the wild/we can't reproduce it anymore?

Comment 22

[Scoobidiver (away)]

The latest crash occurred in 14.0a2/20120506. Closing as WFM