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

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
ImageLib
--
minor
VERIFIED FIXED
14 years ago
12 years ago

People

(Reporter: Glenn Randers-Pehrson, Assigned: Biesinger)

Tracking

Trunk
mozilla1.8beta2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
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.
(Reporter)

Comment 3

14 years ago
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

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

Comment 5

14 years ago
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

14 years ago
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.
(Reporter)

Comment 7

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

Comment 8

14 years ago
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.
Attachment #152889 - Attachment is obsolete: true
(Reporter)

Comment 9

14 years ago
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.

Comment 10

14 years ago
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.
Summary: imgLoader sniffs for unsupported MIME types → Non distributed image formats opens many mozilla windows until crashed
(Reporter)

Comment 11

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

Updated

14 years ago
Attachment #152994 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #152994 - Flags: review?(neil.parkwaycc.co.uk)

Updated

14 years ago
Attachment #152994 - Flags: superreview?(tor)
Attachment #152994 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #152994 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152994 - Flags: review?(bsmedberg)

Comment 12

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

looks good to me
Attachment #152994 - Flags: review?(bsmedberg) → review+

Comment 13

14 years ago
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.
(Reporter)

Comment 14

14 years ago
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

14 years ago
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.
Attachment #152994 - Flags: review+ → review-
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

13 years ago
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.
(Reporter)

Comment 18

13 years ago
WFM, per comment #17.  Resolving INVALID.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → INVALID
Attachment #152994 - Flags: superreview?(tor)
I'm reopening this bug....  Just disabling image formats at compile time should
easily enable you to reproduce this bug.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Reporter)

Comment 20

13 years ago
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?
(Reporter)

Comment 22

13 years ago
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.
Status: REOPENED → ASSIGNED
(Reporter)

Updated

13 years ago
Attachment #152993 - Attachment is obsolete: true
(Reporter)

Comment 23

13 years ago
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.
Attachment #152994 - Attachment is obsolete: true
(Reporter)

Comment 24

13 years ago
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.

Comment 26

13 years ago
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...

Comment 28

13 years ago
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...
Summary: Non distributed image formats opens many mozilla windows until crashed → Non distributed image formats opens many mozilla windows until crashed (infinite/eternal window opening loop)
Created attachment 173746 [details] [diff] [review]
patch (checked in)

don't open windows for unsupported formats.
Assignee: glennrp → cbiesinger
Attachment #173746 - Flags: review?(bzbarsky)
(Reporter)

Comment 30

13 years ago
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?
Attachment #173746 - Flags: review?(bzbarsky) → review+

Comment 32

13 years ago
Created attachment 173795 [details] [diff] [review]
equivalent for firefox

Updated

13 years ago
Blocks: 258511
Attachment #173746 - Flags: superreview?(tor)

Updated

13 years ago
Attachment #173746 - Flags: superreview?(tor) → superreview+

Updated

13 years ago
Attachment #173795 - Flags: review?(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
Attachment #173795 - Flags: review?(bzbarsky) → review+

Updated

13 years ago
Attachment #173795 - Flags: superreview?(darin)
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
Attachment #173746 - Attachment description: patch → patch (checked in)
Target Milestone: --- → mozilla1.8beta2

Comment 35

13 years ago
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
Attachment #173795 - Flags: superreview?(darin) → superreview+

Comment 36

13 years ago
Firefox portion checked in with bz and darin's comments addressed.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.