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.
Created attachment 152889 [details] [diff] [review] 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.
>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.
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.
You should probably change the .ART removal to an ifdef since Netscape ships with .ART support...
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.
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.
Pav: Should there be an entry for *.art (image/x-jg), then, in mozilla/xpfe/components/build/nsModule.cpp ?
Created attachment 152993 [details] [diff] [review] patch imgLoader.cpp sniffer code keeping *.art Re: comment #4, this patch keeps ART sniffing as an #ifdef'ed out block.
Created attachment 152994 [details] [diff] [review] 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.
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.
Has this bug been observed on any platform besides OS/2?
Comment on attachment 152994 [details] [diff] [review] Patch nsModule to register only supported image formats looks good to me
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.
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 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.
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.
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.
WFM, per comment #17. Resolving INVALID.
I'm reopening this bug.... Just disabling image formats at compile time should easily enable you to reproduce this bug.
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
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?
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 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.
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).
(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.
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).
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...
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...
Created attachment 173746 [details] [diff] [review] patch (checked in) don't open windows for unsupported formats.
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 on attachment 173746 [details] [diff] [review] patch (checked in) r=bzbarsky, but could you fix the browser/ code too, please?
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 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 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
Firefox portion checked in with bz and darin's comments addressed.