Closed
Bug 50782
Opened 24 years ago
Closed 23 years ago
ILTRACE core dump in gif.cpp due to LOCAL il_log_module var
Categories
(Core :: Graphics: ImageLib, defect, P1)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: harbaugh, Assigned: cls)
References
Details
Attachments
(5 files)
236.67 KB,
text/plain
|
Details | |
469 bytes,
patch
|
Details | Diff | Splinter Review | |
1.56 KB,
patch
|
Details | Diff | Splinter Review | |
1.84 KB,
patch
|
Details | Diff | Splinter Review | |
4.46 KB,
patch
|
Details | Diff | Splinter Review |
(NOTE: proposed fix is included) 'il_log_module' is re-declared locally in gif.cpp and set to NULL. This causes an attempt to dereference a NULL pointer in the ILTRACE expansion (files gif.cpp, jpeg.cpp and ipng.cpp). Hence the program segfaults before displaying a window. PROPOSED FIX: remove lines 80-81 of gif.cpp, which are actually a *LOCAL* redeclaration of variables that should be GLOBAL 80: int il_debug; 81: PRLogModuleInfo *il_log_module = NULL; The 'true' declaration/initialization of these variables occurs in ilclient.cpp. Similar lines should be removed from jpeg.cpp and ipng.cpp line 80 factors into this because the local 'il_debug' is not initialized and hence can contain random junk ========= analysis =========== Debugger shows that SEGV occurs upon reaching line 859 of gif.cpp: % ./mozilla -g -d ladebug ./run-mozilla.sh -g -d ladebug ./mozilla-bin MOZILLA_FIVE_HOME=. LD_LIBRARY_PATH=.:/usr/local/fbscapp/inst/gtk128/lib:/usr/local/fbscapp/inst/ORBit051/lib:/usr/local/fbscapp/inst/glib128/lib:/usr/shlib:/usr/ccs/lib:/usr/lib/cmplrs/cc:/ usr/lib:/var/shlib LIBRARY_PATH=. SHLIB_PATH=. LIBPATH=. ADDON_PATH=. MOZ_PROGRAM=./mozilla-bin MOZ_TOOLKIT= moz_debug=1 moz_debugger=ladebug /usr/bin/ladebug ./mozilla-bin Welcome to the Ladebug Debugger Version 4.0-62 (built Jun 23 2000) ------------------ object file name: ./mozilla-bin core file name: Warning: cannot open core file : Reading symbolic information ...done (ladebug) use ../../modules/libimg/gifcom Directory search path for source files: . ../../modules/libimg/gifcom (ladebug) run nsNativeComponentLoader: autoregistering begins. nsNativeComponentLoader: autoregistering succeeded nNCL: registering deferred (0) GFX: dpi=96 t2p=0.0666667 p2t=15 depth=16 WEBSHELL+ = 1 Initialized app shell component {18c2f989-b09f-11d2-bcde-00805f0e1353}, rv=0x00000000 Initialized app shell component {33e569b0-40f8-11d4-9a41-000064657374}, rv=0x00000000 ProfileName : default ProfileDir : /users/primgr/harbaugh/.mozilla/default CSSLoaderImpl::LoadAgentSheet: Load of URL 'file:///users/primgr/harbaugh/.mozilla/default/chrome/user.css' failed. Error code: 16389 WEBSHELL+ = 2 Enabling Quirk StyleSheet Note: verifyreflow is disabled Note: styleverifytree is disabled Note: frameverifytree is disabled Enabling Quirk StyleSheet WARNING: chrome: failed to get base url for chrome://necko/locale/necko.properties -- using wacky default, file nsChromeRegistry.cpp, line 507 Obtained name of Personal Toolbar from bookmarks string bundle. Start reading in bookmarks.html Finished reading in bookmarks.html (78125 microseconds) WEBSHELL+ = 3 Thread received signal SEGV stopped at [int il_gif_write(il_container*, const PRUint8*, int32):859 0x3ffbf9140f8] 859 ILTRACE(9,("il:gif: state %d len %d buf %u p %u q %u ep %u", Preprocessing gif.cpp and examining the output shows *TWO* declarations of il_log_module. The first is an 'extern' from if_struct.h; the second is local (line 81 of gif.cpp) and sets il_log_module to NULL. Line 859 expands to { if(il_debug>1) {do { if (((il_log_module)->level >= (1))) { PR_LogPrint ("il:gif: ... where the attempt to dereference (il_log_module)->level SEGFAULTS since the local il_log_module overrides the global.
I did test the proposed fix and it works. The only question is whether it will adversely affect other platforms.
Toni, I am attaching the preprocessed output of gif.cpp that you asked me for. This is with the 6.1 compiler under Tru64 4.0D.
pnunn: You look like the person who gets to approve this change if you like it, and Blame says its your code anyway so thats even better. I asked for a reviewer on the newsgroup, so if they say it looks good and you say it looks good I will check it in.
Comment 6•24 years ago
|
||
This is a slam dunk. I can review and approve, be good if pnunn gave r= too. Please get this in before any branch is cut from the trunk. /be
Status: UNCONFIRMED → NEW
Ever confirmed: true
I would like to see the lines removed as originally proposed instead of littering libimg with "#if 0" junk. Clean sweep alternative attached.
Brendan, can you review/approve tor's patch. He found the same problem in several other files.
Assignee | ||
Comment 10•24 years ago
|
||
We also need to have these global symbol conflicts resolved for static components (bug 46775) to work.
Blocks: 46775
Comment 11•24 years ago
|
||
r: pnunn If someone else can check this in. I am swamped. And some of you Mozillittes can check in easier than I, a dreaded Netscapian, can.
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Comment 12•24 years ago
|
||
a=brendan@mozilla.org, no brainer. /be
Comment 13•24 years ago
|
||
Its already in my tree. Ill check it (tor's patch) in as soon as the tree opens.
Comment 14•24 years ago
|
||
much thanks. -p
Comment 15•24 years ago
|
||
Its checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 16•24 years ago
|
||
This patch breaks the mac. Should I just #ifdef the code so that its only there for that platform?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•24 years ago
|
||
Attaching new fix
Comment 18•24 years ago
|
||
Comment 19•24 years ago
|
||
breandan || pnunn, Could one of you review this new patch?
Comment 20•24 years ago
|
||
Hmmm. We're trying to make these local versions of the symbols go away, right? If they can't go away on Mac, then we've done something wrong somewhere. This patch may be good enough to get the work in for the other platforms, but to me it looks like it means there's still more work to be done on Mac with respect to this bug. I can only r= this patch on the condition that we're still working on the right completely cross-platform fix.
Comment 22•24 years ago
|
||
Looks like these changes were checked in already ... and they break the Win32 debug builds. With undefined symbols for il_log_module and il_debug. You have to also undefine the ILTRACE() macro to get things going again.
Comment 23•24 years ago
|
||
As sheriff, I have backed out the changes, caused build bustage on win32, also I think this can be done xp
Comment 24•24 years ago
|
||
The phone company dug through DSL line the other day, so I have been off the internet for a few days. I was able to read my email this morning only to discover that I broke the debug windows builds on the 27th while trying to fix bug 50872. I watched tinderbox after I did that checkin, and they all stayed green so I was quite supprised this morning to learn that I had broken things. My guess is that both the windows tinderbox machines are running optimized builds? In any event, after the tinderboxes went green I left, and was not available to help sort out the bustage I had caused. I am truly sorry for causing problems. This was my second attempt at fixing 50872, and given that I have broken some platform each time I am now quite scared of trying again. I am only able to test my code under Linux, and that is obviously not sufficient for this code. I am going to see if I can find someone at NS who has access to all 3 platforms to take ownership of this bug.
Comment 25•24 years ago
|
||
Why not just remove those il_debug, il_log_module and ILTRACE from gif.cpp, ipng.cpp, jpeg.cpp. We just lost some debug information. That should solve all the problem. If somebody believe it is really necessary to have those information, we may have to define local version in different names, like ipng_log_module, ipng_debug, IPNG_TRACE. For those small modules, I doubt its necessity.
Comment 26•24 years ago
|
||
*** Bug 62024 has been marked as a duplicate of this bug. ***
Comment 27•24 years ago
|
||
*** Bug 67865 has been marked as a duplicate of this bug. ***
Comment 28•24 years ago
|
||
Whats the progress on this bug were still seeing people having this crash on them.
Comment 29•24 years ago
|
||
There has been no progress as far as I know. We know whats going on, and several proposals to fix this have been put on the table. Someone with some athority (pnunn?) needs to pick one. FWIW, I like Shanjian Li's proposal the best. Lets just remove this code. We dont need it and its causing problems.
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
Taking bug.
Assignee: pnunn → cls
Status: REOPENED → NEW
Keywords: mozilla0.9
OS: OSF/1 → All
Priority: P3 → P1
Hardware: DEC → All
Target Milestone: Future → mozilla0.9
Comment 32•23 years ago
|
||
Verified 03/18/01 ptach on Mac debug for gif, jpg, mng and png decoder projects as wells as libimg project. Still need verification on opt build
Assignee | ||
Comment 33•23 years ago
|
||
Verified debug & opt on win32.
Comment 34•23 years ago
|
||
sr=tor
Comment 35•23 years ago
|
||
r=pavlov
Assignee | ||
Comment 36•23 years ago
|
||
Patch checked in. Marking fixed.
Status: NEW → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•