6553 - Image badge icons don't show on Mac (LoadIconImage must do an asynchronous load)

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Eli Goldberg]

* TITLE/SUMMARY
[PP] Broken image icon only displayed on Win32

(I thought I wrote this up earlier, but can't find a bug report...)

* STEPS TO REPRODUCE
0) Launch Apprunner
1) View http://www.prometheus-music.com/gecko/bug694.html (a page with a link to
a broken image, with pre-specified dimensions; HTML snippet below)

* RESULT
 - What happened

	On Win32, a box appears, containing an image icon (x86/res/gfx/icon_0.gif)
is displayed in the upper-left hand corner of the rectangle, followed by the ALT
text..

	On Linux and Mac OS, the box appears, but lacking the image icon.

 - What was expected

	Consistency cross-platform, or at least a reason for the difference. ;)

* REGRESSION

 - Occurs On
        Win32 Apprunner (5.15.99 AM optimized build [NT 4, Service Pack 3])

 - Doesn't Occur On
        Mac OS Apprunner (5.15.99 AM optimized build)
        Linux Apprunner (5.15.99 AM optimized build)

* HTML SNIPPET

<img src="bla.gif" width=200 height=200 alt="blu">

* CONFIGURATIONS TESTED

- [Mac] Power Mac 8500/120 (233 Mhz 604e), 64 MB RAM (VM on; 1 MB of VM used),
1024x768 (Thousands of Colors), Mac OS 8.6

- [Win32] Vectra VL (233 Mhz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP3.

- [Linux] Vectra VL (266 Mhz P2), 96 MB RAM.

Comment 1

[Hixie (not reading bugmail)]

This bug is one of many related to alternate text of images. All these bugs
have been marked. To find related bugs, search the description field for the
string "[ALT]".

Comment 2

[Patrick C. Beard]

This is explicitly unsupported on XP_MAC and XP_UNIX. Take a look at:

http://lxr.mozilla.org/seamonkey/source/gfx/src/nsDeviceContext.cpp#345

It unconditionally returns NS_ERROR_FAILURE on XP_MAC and XP_UNIX when trying to
load the broken icon image. Annoying.

Comment 3

[Eli Goldberg]

Thank you, Patrick!

<pretending to understand the source code> So...should this be deferred until
Necko, or will netlib continue not have a separate thread on Unix/Mac OS after
the rewrite?

Comment 4

[Hixie (not reading bugmail)]

As per comments above, updating the bug summary. I have also set the component
to NetLib, but have not reassigned the bug.

The code snippet in question is the following:

 NS_IMETHODIMP DeviceContextImpl::LoadIconImage(PRInt32 aId, nsIImage*& aImage)
 {
   // XXX synchronous image loading doesn't work on unix or mac yet
   // because netlib is not in its own thread.
 #if defined(XP_UNIX) || defined(XP_MAC)
   return NS_ERROR_FAILURE;
 #endif
 // ...etc...
 }

I am quoting it here because by the time this is fixed, line numbers are almost
guarenteed to have changed.

(Note that although the _broken_ image icon should never be used -- see bug
5764 and bug 3268 -- the "downloading" image icon will still be wanted, so this
method is not going to be decomissioned and this bug still needs to be fixed.)

The following test page may also be useful:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/IMGalt2.html

Comment 5

[leger]

Changing all Networking Library/Browser bugs to Networking-Core component for
Browser.

Occasionally, Bugzilla will burp and cause Verified bugs to reopen when I do
this in a bulk change.  If this happens, I will fix. ;-)

Comment 6

[Kevin McCluskey (gone)]

Now that Necko has landed, need to remove the
 #if defined(XP_UNIX) || defined(XP_MAC)
   return NS_ERROR_FAILURE;
 #endif
and try it on Unix and Mac. I

Comment 7

[Kevin McCluskey (gone)]

Ramiro, could you try removing the XP_UNIX check and run on Linux? Now that
NetLib is landed, supposedly LoadIconImage should work.

Don and Patric could you do the same on Mac?

NS_IMETHODIMP DeviceContextImpl::LoadIconImage(PRInt32 aId, nsIImage*& aImage)
 {
   // XXX synchronous image loading doesn't work on unix or mac yet
   // because netlib is not in its own thread.
 #if defined(XP_UNIX) || defined(XP_MAC)
   return NS_ERROR_FAILURE;
 #endif
 // ...etc...
 }

Comment 8

[Kevin McCluskey (gone)]

Moving to M11

Comment 9

[Kevin McCluskey (gone)]

Tried removing the code which returns with NS_ERROR_FAILURE on Linux and Mac.

The testcases
http://www.prometheus-music.com/gecko/bug694.html,
http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/IMGalt2.html

appears the same on Linux and WIN32.
The Mac crashes.

Comment 10

[Kevin McCluskey (gone)]

Removed #ifdef XP_UNIX in DeviceContextImpl::LoadIconImage which prevented it
from loading an icon under Unix. Now that necko has landed LoadIconImage is
working on Linux.

Mac is still not working.
Don, I'm reassigning to you for the Mac issue.

Comment 11

[leger]

Bulk move of all Networking-Core (to be deleted component) bugs to new
Networking component.

Comment 12

[dcone (gone)]

this does not work on Windows or the Mac.  I think this is an XP problem at the
moment.  Kevin do you know who this belongs to, to get this broken image to
load.

Comment 13

[Kevin McCluskey (gone)]

It is working on WIN32.

nsDeviceContext.cpp still contains a XP_MAC define which always returns
NS_ERROR_FAILURE.

NS_IMETHODIMP DeviceContextImpl::LoadIconImage(PRInt32 aId, nsIImage*& aImage)
{
  // XXX synchronous image loading doesn't work on mac yet
#if defined(XP_MAC) || defined(XP_BEOS)
  return NS_ERROR_FAILURE;
#endif
  nsresult  result;

Comment 14

[Kevin McCluskey (gone)]

*** Bug 24559 has been marked as a duplicate of this bug. ***

Comment 15

[Simon Fraser [no longer active]]

I tried removing the defined(XP_MAC), and things seemed to work just fine on Mac. 
I got badge icons for broken images, and live was relatively good.

Comment 16

[Simon Fraser [no longer active]]

kmcclusk/dcone: are we ok to enable this on Mac? What more testing do we need?

Comment 17

[dcone (gone)]

Took out the ifdef

Comment 18

[Chris Petersen]

Ian, 

Eli suggested that I reassign this report to you since you are QA owner for ALT 
text-like bugs.

Comment 19

[Hixie (not reading bugmail)]

Ok, Eli, since you suggested to petersen that I should QA verify this bug, I'm
going to have to take you up on your offer of cross-platform verifications! :-)

Could you give me a screenshot of how this page:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/IMGalt2.html
...looks, after 10 seconds, using the latest Mac builds?
(The time limit is important, after 60 seconds the page should change quite a
bit because that's how long it will take for the images to register as broken.)

If you have access to a Unix machine (Linux, Solaris, whatever) then an
equivalent screenshot on those platforms would be nice too. 

Cheers!

Comment 20

[Eli Goldberg]

Attachment: results of evil test on Mac OS

Comment 21

[Eli Goldberg]

Ian, that's a long page with a lot of test cases; I don't think it'll fit on my 
monitor. ;) 

Are there particular sections that's you'd like screen shots of? I've attached 
the top test case from the 2000012808 Mac OS build.

Will find out how to do a Linux screen capture and attach (unless anyone else 
would like to do so sooner.)

Comment 22

[pnunn]

Eli:
I believe Simon file a bug or two that
relates to the icon problem. You might
want to scan those 2 as well. 
Wish I could remember the bug #'s for you. :(

-p

Comment 23

[Simon Fraser [no longer active]]

The only other bug related to this problem was that the icon_0.gif/icon_1.gif 
files weren't being installed on Mac. I fixed that.

Comment 24

[Hixie (not reading bugmail)]

Eli: It's the top test case which I meant, so that's fine, thanks.
Looks fixed to me, unless someone sees another problem.
VERIFIED.

Comment 25

[Simon Fraser [no longer active]]

Reopening. This change on Mac has caused a slew of nasty bugs

Comment 26

[Simon Fraser [no longer active]]

So the change to fix this on Mac has caused the following bugs which I'm marking 
dependent on this.

Comment 27

[dcone (gone)]

I don't understand.  When I run the test case this bug is fixed.
What are the other bugs? Did this cause those bugs?  If not, then this bug 
should be closed.  If it did then I will put the ifdef back in and research 
whats is going on, or give this bug to the person who owns these other issues.

Comment 28

[Simon Fraser [no longer active]]

OK, I'm going to turn off LoadIconImage() on Mac again, until we can resolve 
these problems.

The main issue here is that it is illegal to do a synchronous load of the GIF 
file on the main thread. Trying to do this causes event and timing problems which 
manifest themselves in the problems observed.

Loading the GIF file must be asynchronous. Valeski has a necko interface that is 
soon to be checked in that makes such loading easier, by responding to a callback 
that gets called when all the data is available.

Comment 29

[Simon Fraser [no longer active]]

Adjusting summary. The interface you need, when valeski checks it in, is 
nsIStreamLoader.

ImageNetContextSyncImpl should probably be removed entirely while this is fixed.

Comment 30

[dcone (gone)]

Simon.. do you want this bug.. you seem to know more about the correct solution 
than me.

Comment 31

[Simon Fraser [no longer active]]

No, I don't want this bug. You need to come up with a way to load the image icon 
GIF asynchronously, on all platforms.

Comment 32

[Simon Fraser [no longer active]]

Reassign back. This ain't my bag, baby.

Comment 33

[Judson Valeski]

There are two async load options (nsIStreamLoader, and nsIStreamListener).

Comment 34

[Simon Fraser [no longer active]]

Nominating for nsbeta2. It is a regression that image badges don't show for 
unloaded and broken image on Mac. This is also a platform parity issue.

Comment 35

[Simon Fraser [no longer active]]

valeski: is the kind of sync loading done in mozilla/gfx/
nsImageNetContextSync.cpp, ImageNetContextSyncImpl::GetURL() OK to do now? Or is 
this still bad?

Comment 36

[Judson Valeski]

if you want to use nsImageContextSync loading to solve this bug, you'll 
need to wait for 22103 to be fixed. If you're sure you're only loading files 
from disk, you *might* be able to get away w/ nsImageContextSync without the 
22103 fix (note: I have a patch in 22103 that *should* make everything ok).

Comment 37

[Peter Trudelle]

[nsbeta2-]

Comment 38

[Simon Fraser [no longer active]]

I turned on badge icons on Mac in nsDeviceContext, and they show up before images 
load just fine. So the sync loading problems seem to be better. I think we can 
just change that #ifdef.

Comment 39

[dcone (gone)]

This bug has been marked future because we have determined that it is not 
critical for netscape 6.0.  If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 40

[Simon Fraser [no longer active]]

Taking the bug. I've been running with this on in my tree for ages now, and 
everything works fine.

Comment 41

[rubydoo123]

setting to nsbeta3

Comment 42

[rubydoo123]

setting to nsbeta3+, who reviewed?

Comment 43

[rubydoo123]

adding brackets to status whiteboard

Comment 44

[Simon Fraser [no longer active]]

Fixed, yeah.

Comment 45

[leger]

Updating QA Contact.

Comment 46

[Paul Wyskoczka]

Clicking on this link http://www.prometheus-music.com/gecko/bug694.html results 
in the text "blu" showing up in the left hand corner. Nothing else appears.
Reopening

Comment 47

[Paul Wyskoczka]

This occurs in mac build 2000090508 and Windows build 2000091104 and Linux build 
2000091106

Comment 48

[Simon Fraser [no longer active]]

The image badge icon shows correctly while the image is loading (it flashes up on 
Mac). The broken image icon does not show once the image has been found to be 
missing; that's an old layout bug, bug 41924.

Comment 49

[Paul Wyskoczka]

Saw it on mac. Verifying in build 2000091120