76152 - Can't launch if prefs.js contains "intl.charset.detector"

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Koike Kazuhiko]

Recent Linux builds can't be launched if prefs.js contains:

user_pref("intl.charset.detector", "ja_parallel_state_machine");

Comment 1

[Frank Tang]

shanjian- please take a look at this. 
cc bstell to support shanjian since he also running linux build. 

Comment 2

[Shanjian Li]

This need to be verified. It does not happen on HP. I am building a linux build now.

Comment 3

[Shanjian Li]

It works just fine for me. If problem still happens to you, please reopen
the bug and provide more information.

Comment 4

[Koike Kazuhiko]

Shanjian, if you are using Linux build from CVS, try the following
setting.

export MOZILLA_OFFICIAL=1
export BUILD_OFFICIAL=1

($HOME/.mozconfig)
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --enable-optimize
ac_add_options --without-jpeg
ac_add_options --without-zlib
ac_add_options --without-png

Nightly build from 12 Apr have this bug.

Comment 5

[kill this account]

With this line added to my prefs.js my cvs/debug build of 04/17/2001 and it 
launched without any problem. I saw messages from the auto detector so I'm sure 
I added the line to the right file.

I'll try setting the options Koike suggested and building.

Comment 6

[kill this account]

I built with the suggested configure setting and it crashed.

I built with debug and it starts without problem

  #ac_add_options --disable-debug

Comment 7

[Roy Yokoyama]

I made bunch of changes to MetaCharset Detector recently.  This may be caused
by my changes.   Is this only happening on Linux?  Any other platforms?
I am cc'ing other QAs.

Comment 8

[Shanjian Li]

Yes, this happens on Linux. 

I tried to backout your changes on my local tree, and the problem 
disappeared. So I am pretty sure that some of your changes caused this 
problem. I will leave this problem to you. Ask help from bstell if you
need.

Comment 9

[kill this account]

Program received signal SIGSEGV, Segmentation fault.
__pthread_mutex_lock (mutex=0x380448) at mutex.c:82

Comment 10

[kill this account]

mozilla build, source=04/17/2001. with settings from 
"Additional Comments From KOIKE Kazuhiko 2001-04-17 14:19"

Profile dialog came up, I hit return
it crashed

(gdb) bt
#0  0x402480e4 in chunk_free (ar_ptr=0x402dcd60, p=0x83291a0) at malloc.c:3100
#1  0x40247fba in __libc_free (mem=0x83291a8) at malloc.c:3023
#2  0x401b5d9a in PR_Free () from
/builds/bstell/mojo2/mozilla/dist/bin/libnspr4.so
#3  0x400d2026 in nsMemoryImpl::Free () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#4  0x400d26df in nsMemory::Free () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#5  0x400f6a4e in nsStr::Free () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#6  0x400f5d33 in nsStr::Destroy () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#7  0x400f9a6a in nsString::~nsString () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#8  0x40a26d5b in CStartToken::~CStartToken () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libhtmlpars.so
#9  0x40a096ee in CWellFormedDTD::BuildModel () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libhtmlpars.so
#10 0x409ff5e9 in nsParser::BuildModel () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libhtmlpars.so
#11 0x409ff3ff in nsParser::ResumeParse () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libhtmlpars.so
#12 0x409ffea3 in nsParser::OnDataAvailable () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libhtmlpars.so
#13 0x409229d6 in nsJARChannel::OnDataAvailable () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libnecko.so
#14 0x408e02fe in nsOnDataAvailableEvent::HandleEvent () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libnecko.so
#15 0x408dfac1 in nsARequestObserverEvent::HandlePLEvent () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libnecko.so
#16 0x400ca108 in PL_HandleEvent () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#17 0x400c9ff9 in PL_ProcessPendingEvents () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#18 0x400cb19c in nsEventQueueImpl::ProcessPendingEvents () from
/builds/bstell/mojo2/mozilla/dist/bin/libxpcom.so
#19 0x4051c780 in event_processor_callback () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libwidget_gtk.so
#20 0x4051c48b in our_gdk_io_invoke () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libwidget_gtk.so
#21 0x406bf089 in g_io_unix_dispatch (source_data=0x83328e0,
current_time=0xbffff550, user_data=0x8293ef0) at giounix.c:135
#22 0x406c0846 in g_main_dispatch (dispatch_time=0xbffff550) at gmain.c:656
#23 0x406c0e73 in g_main_iterate (block=1, dispatch=1) at gmain.c:877
#24 0x406c102c in g_main_run (loop=0x834cff8) at gmain.c:935
#25 0x405e1a4b in gtk_main () at gtkmain.c:476
#26 0x4051cd2d in nsAppShell::Run () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libwidget_gtk.so
#27 0x404fb5c7 in nsAppShellService::Run () from
/builds/bstell/mojo2/mozilla/dist/bin/components/libnsappshell.so
#28 0x0804d7bf in main1 ()
#29 0x0804e1bc in main ()
#30 0x402069cb in __libc_start_main (main=0x804e05c <main>, argc=1,
argv=0xbffff7e4, init=0x804a100 <_init>, fini=0x804fe78 <_fini>,
rtld_fini=0x4000ae60 <_dl_fini>, stack_end=0xbffff7dc) at
../sysdeps/generic/libc-start.c:92

(gdb) 

Comment 11

[Roy Yokoyama]

Attachment: Using NS_IMPL_QUERY_INTERFACE2

Comment 12

[Roy Yokoyama]

We finally figure this out.  Thanks to bstell.

It's the #ifdef IMPL_NS_IPARSERFILTER.
IMPL_NS_IPARSERFILTER was not defined in nsCharDetModule.cpp where
nsDetectionAdaptor was constructed.

To only fix this bug, we can define IMPL_NS_IPARSERFILTER in
nsDetectionAdaptor.h.

However, I attached the patch to remove the IMPL_NS_IPARSERFILTER
and did a little clean up.

bstell: can you /r= ?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz for 0.9 on behalf of drivers

But you have to get sr from someone, AND you should also make sure it builds and 
runs on Mac/Win/Linux if you possibly can

Comment 14

[Roy Yokoyama]

roc: thanks. 
I have bstell helped me compiled under Linux.
Can I got someone tested under Mac?

brendan: can you /sr=?

Comment 15

[Brendan Eich [:brendan]]

1.  Use NS_IMPL_ISUPPORTS2 now that you're not customizing QI.

2.  Don't use 3-space indentation here:

-      if(NS_SUCCEEDED(rv)) {
-         rv = mObserver->Init(aWebShellSvc, aDocument, 
-              aParser, aCharset, aCommand);
+         nsCOMPtr<nsICharsetDetectionObserver> aObserver;
+         nsresult rv = NS_OK;
+         mObserver = new nsMyObserver(); // weak ref to it, release by charset
detector
+         if(nsnull == mObserver)
+            return NS_ERROR_OUT_OF_MEMORY;
+         NS_IF_ADDREF(mObserver);
+
+         rv =
mObserver->QueryInterface(NS_GET_IID(nsICharsetDetectionObserver),
+                                  getter_AddRefs(aObserver));
+        
          if(NS_SUCCEEDED(rv)) {
-            rv = aDetector->Init(aObserver);
-         }

Make the code follow the Emacs modeline comment at line 1, or vice versa.  
Also, don't name a local variable aObserver -- that's the convention for formal
arguments.  Just observer will do.

Control flow is unnecessarily branchy here:

+            if(NS_SUCCEEDED(rv)) {
+               rv = aDetector->Init(aObserver);
+            }
+
+            if(NS_FAILED(rv))
+                 return rv;
+
+            NS_IF_ADDREF(aDetector);
+            mDetector = aDetector;

Test NS_FAILED and return early first, then don't retest success, just do the
Init and mDetector = aDetector; in straight line code.  I know you moved this
code, but no time like the present to clean it up.

3.  Now that mDetector is an nsCOMPtr, you must not NS_IF_ADDREF(aDetector) (and
you need not initialize it in the class ctor, as the patch does not -- good).

/be

Comment 16

[Jon Rubin]

I just checked this in MacOS 9.1 using build 2001041908.  Launched fine with 
"intl.charset.detector".

Comment 17

[Brendan Eich [:brendan]]

I hope it's clear that my point 3 identifies a leak in the patch.  New patch
coming soon?

/be

Comment 18

[Roy Yokoyama]

Attachment: As per brendan's suggestion.

Comment 19

[Brendan Eich [:brendan]]

Ah, I had thought that mObserver was weak as the comment said, but it's not: it
starts life null, may be set with an ADDREF (hence strong if non-null), and is
dropped in the dtor using NS_IF_RELEASE.  So instead of:

+    nsCOMPtr<nsICharsetDetectionObserver> observer;
+    nsresult rv = NS_OK;
+    mObserver = new nsMyObserver(); // weak ref to it, release by charset
detector
+    if(nsnull == mObserver)
+       return NS_ERROR_OUT_OF_MEMORY;
+    NS_IF_ADDREF(mObserver);
+
+    rv = mObserver->QueryInterface(NS_GET_IID(nsICharsetDetectionObserver),
+                             getter_AddRefs(observer));

just make mObserver be an nsCOMPtr<nsICharsetDetectionObserver> and you can get
rid of most of this code, replacing it with

+    nsMyObserver* observer = new nsMyObserver();
+    if(!observer)
+       return NS_ERROR_OUT_OF_MEMORY;
+    nsresult rv =
observer->QueryInterface(NS_GET_IID(nsICharsetDetectionObserver),
+                        getter_AddRefs(mObserver));

or even (since nsMyObserver inherits singly from nsISupports):

+    mObserver = new nsMyObserver();
+    if(!observer)
+       return NS_ERROR_OUT_OF_MEMORY;

and get rid of the weak ref comment.

There is no need for the else or braces and indentation at

+    } 
+    else {
+      NS_IF_RELEASE(mObserver);
+    }

in the latest patch, but changing mObserver to an nsCOMPtr gets rid of this
error-prone busy work altogether.

/be

Comment 20

[Brendan Eich [:brendan]]

Typo here:

+    mObserver = new nsMyObserver();
+    if(!observer)
+       return NS_ERROR_OUT_OF_MEMORY;

The if condition should of course be (!mObserver).

/be

Comment 21

[kill this account]

compiles on Linux
r=[e-mail]

Comment 22

[Brendan Eich [:brendan]]

Roy pointed out that mObserver needs to point to the nsMyObserver concrete type.
I suggested using nsCOMPtr<nsMyObserver>, but if that doesn't work, then the
patch ("As per brendan's suggestion.") could still be improved:

+         rv = aDetector->Init(observer);
+      }
+      mDetector = aDetector;
+      mDontFeedToDetector = PR_FALSE;
+      return NS_OK;
+    } 
+    else {
+      NS_IF_RELEASE(mObserver);
     }
-    return NS_ERROR_ILLEGAL_VALUE;
+  }
+  return NS_ERROR_ILLEGAL_VALUE;

As I wrote, there's no point in else after return, or the _IF in NS_IF_RELEASE,
but I also noticed that the rv returned by aDetector->Init() is not checked or
returned (also, that line,

+         rv = aDetector->Init(observer);

is still not indented properly).  How about:

+        rv = aDetector->Init(observer);
+        if (NS_SUCCEEDED(rv)) {
+          mDetector = aDetector;
+          mDontFeedToDetector = PR_FALSE;
+          return NS_OK;
+        }
+      }
+    } 
+    NS_RELEASE(mObserver);
+  }
+  return NS_ERROR_ILLEGAL_VALUE;
+}

This way, all error paths once we've ADDREF'd mObserver will fall through to the
NS_RELEASE(mObserver); and then return an error code (not sure if it's always
the best code -- what is the ILLEGAL_VALUE being reported)?

/be

Comment 23

[chris hofmann]

putting this back on the 0.9 radar.

Comment 24

[Roy Yokoyama]

Attachment: Revised: As per brendan's suggestion.

Comment 25

[Roy Yokoyama]

Attachment: Using nsCOMPtr<nsMyObserver>.

Comment 26

[Roy Yokoyama]

brendan: can I get /sr= on either 04/21/01 13:57 or 04/21/01 15:32?
Thanks

Comment 27

[Brendan Eich [:brendan]]

I was waiting for scc to verify that nsCOMPtr<nsMyObserver> was kosher.  I think
it is, and it works.

One more simplification, then I'll sr= -- you don't need the observer local
variable, or the manual QueryInterface call from mObserver to get it.  Just pass
mObserver.get() to aDetector->Init().  No need to QI if you know nsMyObserver
singly-inherits nsICharsetDetectionObserver.  Do that, and
sr=[e-mail] -- then get this in for 0.9.  Thanks,

/be

Comment 28

[Roy Yokoyama]

Attachment: Using nsCOMPtr<nsMyObserver>, remove QI and pass mObserver.get() to aDetector->Init()

Comment 29

[Brendan Eich [:brendan]]

Looks good -- works good too, I hope.  sr=[e-mail]

Comment 30

[Roy Yokoyama]

I run the Purify.  No complains. :) (before the patch, there was a memory 
override error.)

Comment 31

[Roy Yokoyama]

checked-in.

Comment 32

[KANEUCHI Tetsuya]

2001042221/Linux works fine for me. Great!

Comment 33

[Jon Rubin]

Verified on Redhat7.0-ja using 2001052205 build.