250936 - Non distributed image formats opens many mozilla windows until crashed (infinite/eternal window opening loop)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

There is a section of code in modules/libpr0n/src/imgLoader.cpp that checks
"magick numbers" at the beginning of image files to determine the MIME type.
Some of this code checks for an unsupported type (AOL *.art) and other parts
can check for types that the user has decided not to support and has eliminated
from the IMG_DECODERS configuration variable (perhaps via the
--ac_add_options --enable-image-decoders=png,gif,jpeg,xbm .mozconfig
directive that would eliminate BMP support.

Thie problem was first noticed in bug #18574 (MNG support) although it does
not seem to be a MNG-specific bug.  Accordingly, I'm opening this new bug.

It was reported that besides adding or retaining a small amount of code
bloat, this can cause trouble.  For example, removing BMP from the IMG_DECODERS
list, building mozilla, then trying to display a BMP file results in a large
number of mozilla windows being opened.  See bug #18574 comment #458.

There should be suitable #ifdef/#endif blocks in imgLoader.cpp to remove
the checks for unsupported image types.  I'll upload a patch to do this
shortly.

Comment 1

[Glenn Randers-Pehrson]

Attachment: Patch MIME sniffer code in libpr0n/src/imgLoader.cpp

Removes "*.art" sniffing and places #ifdef IMG_BUILD_type/#endif around
code for sniffing other types, so that if a particular type is not supported,
the sniffing code will be #ifdef'ed out.  About the ART format, see
bug #153450.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

>then trying to display a BMP file results in a large
>number of mozilla windows being opened.

I find it hard to believe that this patch has an effect on that. even if so,
that bug likely has a different root cause and should be separately investigated.

Comment 3

[Glenn Randers-Pehrson]

I don't know if it fixes the "many open windows" bug because that bug isn't
exhibited on my platform.  However, the patch removes some unused code,
a.k.a. bloat, which is probably a good thing anyhow.

Comment 4

[Aaron Kaluszka]

You should probably change the .ART removal to an ifdef since Netscape ships
with .ART support...

Comment 5

[Alexander Opitz]

Hi Glenn,

please let the imgLoader.cpp untouched, it's ok to know the mime type for images
that we don't support ;-)
But you can add the ifdefs to
http://lxr.mozilla.org/seamonkey/source/xpfe/components/build/nsModule.cpp#259
and the following lines, cause there the image decoders are registered, also the
ones that aren't compiled. Some lines above it is shown how SVG it does.

Comment 6

[Stuart Parmenter]

Since all of these image decoders can be dropped in at any time, all of the
existing checks are valid and ifdefing them is not.

Comment 7

[Glenn Randers-Pehrson]

Pav: Should there be an entry for *.art (image/x-jg), then, in
mozilla/xpfe/components/build/nsModule.cpp ?

Comment 8

[Glenn Randers-Pehrson]

Attachment: patch imgLoader.cpp sniffer code keeping *.art

Re: comment #4, this patch keeps ART sniffing as an #ifdef'ed out block.

Comment 9

[Glenn Randers-Pehrson]

Attachment: Patch nsModule to register only supported image formats

This patch puts unsupported image formats inside #ifdef IMG_BUILD_format/#endif

blocks.  Peter (or anyone seeing the many-open-windows problem) please test to
see if this fixes the problem.

Comment 10

[Alexander Opitz]

Update summary to the kernel of the problem.

And as told before, let the imgLoader.cpp untouched cause that won't help and
bring us some other side effects.

The nsModule Patch should work and seems to be ok for me.

Comment 11

[Glenn Randers-Pehrson]

Has this bug been observed on any platform besides OS/2?

Comment 12

[Benjamin Smedberg]

Comment on attachment 152994 [details] [diff] [review]
Patch nsModule to register only supported image formats

looks good to me

Comment 13

[Stuart Parmenter]

So it appears to me that whatever bug is trying to be fixed here is being fixed
in completely the wrong way.  It also seems that we aren't even sure if this is
really even a bug.

Comment 14

[Glenn Randers-Pehrson]

pav: right.

r/sr seem premature.  I posted the patches in hopes of hearing back
from Peter whether either would fix the problem that he reported over
in bug #18574.

Comment 15

[Stuart Parmenter]

Comment on attachment 152994 [details] [diff] [review]
Patch nsModule to register only supported image formats

this patch says that we _have_ to register for any type we might ever support. 
that seems completely wrong.

this list should be built dynamically probably using categories.

it seems like this bug should just be closed and another bug to fix the way
this list is built should be filed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I would be quite happy to augment the content handler contractid prefix with a
content handler category (the way we have a URIContentListener category).  If
people feel that's a reasonable way to go, I can post appropriate patches to the
URILoader on whatever bug gets filed.

Comment 17

[Peter Weilbacher]

After being offline for the last two weeks I found out yesterday that the
"many-window-problem" for BMPs and other formats that this bug is about, was a
side-effect of an older version of the MNG patch from bug 18574. Neither a clean
1.7 branch checkout nor a clean checkout with the newest MNG patch (attachment
152566 [details]) exhibit the problem (for other formats than MNG/JNG).

As the (only?) one who saw the problem I recommend to resolve this bug as WFM.

Comment 18

[Glenn Randers-Pehrson]

WFM, per comment #17.  Resolving INVALID.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm reopening this bug....  Just disabling image formats at compile time should
easily enable you to reproduce this bug.

Comment 20

[Glenn Randers-Pehrson]

Well, as I said in comment #3, the bug isn't exhibited on my platform.  Now,
to my knowledge, it isn't exhibited on anyone's, including Peter's.
If someone else does see the many-open-windows problem, please report it here.
Otherwise, if no reports come in during the next week, I'll close this bug
again.

There does seem to remain a problem with opi's patch for MNG/JNG support,
possibly only on OS/2, but that's only a problem for those of us working
on MNG/JNG.

Glenn

Comment 21

[Boris Zbarsky [:bzbarsky]]

Glenn, if you build with:

  --ac_add_options --enable-image-decoders=png,gif

and try to open a jpeg file, you don't get the problem?

Comment 22

[Glenn Randers-Pehrson]

Re comment #21: I got lots of windows opening until I went to the console
window and killed mozilla.  I do not see a problem when I open an HTML page
with JPEGs on it, only if I use the "file:///...opossum.jpg" syntax.  So
I agree that the bug should be reopened.

To be sure of what I was running, I removed all the old *.so and stuff
from previous installations, and did a fresh checkout from trunk (1.8a3),
and rebuilt.

Comment 23

[Glenn Randers-Pehrson]

Comment on attachment 152994 [details] [diff] [review]
Patch nsModule to register only supported image formats

Now that I can observe the bug, I tried both patches and neither changed the
behavior.  I get many windows when I try to view a file:///...jpg

Marked both patches obsolete.

Comment 24

[Glenn Randers-Pehrson]

With the same build (that only supports gif and png) if I try to view
file.tiff, I get a popup window that says "this file is of type image/tiff
and Mozilla does not know how to handle this type of file" or words to
that effect (i.e., proper behavior).

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #24)
> With the same build (that only supports gif and png) if I try to view
> file.tiff, I get a popup window that says "this file is of type image/tiff

well, for TIFF, no content handler is registered.

the reason that a new window is opened is because of
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/src/nsBrowserInstance.cpp#794
since that new window can't handle the content either, uriloader calls into this
code again. so a new window is opened. etc.

Comment 26

[Peter Weilbacher]

What is the reason for opening a new window? As mostly a user of Mozilla I have
never had the experience that a new window could handle content that the old one
could not (or I have not noticed).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Peter, if you click on a link to an html file in a mailnews message, a browser
window should open.  This is the code that handles that...

Comment 28

[Peter Weilbacher]

I see. So to get rid of this unlimited opening of the same type of window, one
need to test if the call for the new window is coming from the same type of
window that would be opened.

Is there such a comparison available for parentWindow vs. newWindow from within
nsBrowserContentHandler::HandleContent()? Despite browsing with lxr for half an
hour I didn't find out myself...

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch (checked in)

don't open windows for unsupported formats.

Comment 30

[Glenn Randers-Pehrson]

Patch id 173746 works for me.  Instead of a horde of windows I get a single
popup that asks what to do with the unknown mime type.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173746 [details] [diff] [review]
patch (checked in)

r=bzbarsky, but could you fix the browser/ code too, please?

Comment 32

[tor]

Attachment: equivalent for firefox

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173795 [details] [diff] [review]
equivalent for firefox

>Index: nsBrowserContentHandler.js
>+      var entry = catMan.getCategoryEntry("Gecko-Content-Viewers",
>+					  contentType);

Fix indent?

r=bzbarsky

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 173746 [details] [diff] [review]
patch (checked in)

Checking in xpfe/browser/src/nsBrowserInstance.cpp;
/cvsroot/mozilla/xpfe/browser/src/nsBrowserInstance.cpp,v  <-- 
nsBrowserInstance.cpp
new revision: 1.273; previous revision: 1.272
done


leaving open for the firefox patch

Comment 35

[Darin Fisher]

Comment on attachment 173795 [details] [diff] [review]
equivalent for firefox

>Index: nsBrowserContentHandler.js

> const nsIPrefBranch          = Components.interfaces.nsIPrefBranch;
> const nsIPrefLocalizedString = Components.interfaces.nsIPrefLocalizedString;
> const nsISupportsString      = Components.interfaces.nsISupportsString;
> const nsIWebNavigation       = Components.interfaces.nsIWebNavigation;
> const nsIWindowMediator      = Components.interfaces.nsIWindowMediator;
> const nsIWindowWatcher       = Components.interfaces.nsIWindowWatcher;
...
>+      var catMan = Components.classes["@mozilla.org/categorymanager;1"]
>+                   .getService(Components.interfaces.nsICategoryManager);

so, this file appears to like to define interfaces up front.
how about doing the same for nsICategoryManager?  then you 
could write:

      var catMan = Components.classes["@mozilla.org/categorymanager;1"]
			     .getService(nsICategoryManager);

which is a bit more readable.


sr=darin with or without this change

Comment 36

[tor]

Firefox portion checked in with bz and darin's comments addressed.