15261 - [DOGFOOD]CRASH when creating new profile

Status: VERIFIED FIXED

(Core Graveyard :: Profile: BackEnd, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

run -installer
hit new to create a new profile
create profile, and hit finish

crash.

here's a patch to fix the problem:
Index: jsstr.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsstr.c,v

retrieving revision 3.12
diff -p -r3.12 jsstr.c
*** jsstr.c     1999/09/28 23:10:55     3.12
--- jsstr.c     1999/09/30 18:00:03
*************** js_GetStringBytes(JSString *str)
*** 2379,2384 ****
--- 2379,2386 ----
      JSHashNumber hash;
      JSHashEntry *he, **hep;

+     if (!str) return NULL;
+
      JS_ACQUIRE_LOCK(deflated_string_cache_lock);

      cache = GetDeflatedStringCache();


here's the stack:

#0  0x400c9896 in js_GetStringBytes (str=0x0) at jsstr.c:2394
#1  0x4007ac12 in js_ReportUncaughtException (cx=0x8381b38) at jsexn.c:655
#2  0x40057daa in JS_CallFunction (cx=0x8381b38, obj=0x841b160, fun=0x8537250,
argc=1, argv=0xbfffeba4, rval=0xbfffea8c) at jsapi.c:2652
#3  0x403df04a in nsJSContext::CallFunction (this=0x8381618, aObj=0x841b160,
aFunction=0x8537250, argc=1, argv=0xbfffeba4, aBoolResult=0xbfffeaf4) at
nsJSEnvironment.cpp:228
#4  0x40417585 in nsJSEventListener::HandleEvent (this=0x8537488,
aEvent=0x8579e08) at nsJSEventListener.cpp:103
#5  0x40c33840 in nsEventListenerManager::HandleEvent (this=0x8537088,
aPresContext=@0x821e9b8, aEvent=0xbfffee98, aDOMEvent=0xbfffee40, aFlags=7,
aEventStatus=@0xbffff1a8) at nsEventListenerManager.cpp:646
#6  0x408a50cd in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/components/librdf.so
#7  0x40c378c2 in nsEventStateManager::CheckForAndDispatchClick (this=0x8535d10,
aPresContext=@0x821e9b8, aEvent=0xbffff288, aStatus=@0xbffff1a8) at
nsEventStateManager.cpp:947
#8  0x40c36535 in nsEventStateManager::PostHandleEvent (this=0x8535d10,
aPresContext=@0x821e9b8, aEvent=0xbffff288, aTargetFrame=0x851b828,
aStatus=@0xbffff1a8, aView=0x8291548) at nsEventStateManager.cpp:418
#9  0x40c7d18d in PresShell::HandleEvent (this=0x82b09e0, aView=0x8291548,
aEvent=0xbffff288, aEventStatus=@0xbffff1a8) at nsPresShell.cpp:2091
#10 0x41019499 in nsView::HandleEvent (this=0x8291548, event=0xbffff288,
aEventFlags=28, aStatus=@0xbffff1a8, aHandled=@0xbffff14c) at nsView.cpp:827
#11 0x41023b23 in nsViewManager::DispatchEvent (this=0x84f4a30,
aEvent=0xbffff288, aStatus=@0xbffff1a8) at nsViewManager.cpp:1662
#12 0x410175f4 in HandleEvent (aEvent=0xbffff288) at nsView.cpp:62
#13 0x40529a20 in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#14 0x405297bc in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#15 0x40529ae0 in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#16 0x4052ab7d in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#17 0x4052b746 in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#18 0x406b879d in ?? () from /usr/lib/libgtk-1.2.so.0
#19 0x40680037 in ?? () from /usr/lib/libgtk-1.2.so.0
#20 0x4067f52f in ?? () from /usr/lib/libgtk-1.2.so.0
#21 0x4067d800 in ?? () from /usr/lib/libgtk-1.2.so.0
#22 0x406b05b8 in ?? () from /usr/lib/libgtk-1.2.so.0
#23 0x406551a2 in ?? () from /usr/lib/libgtk-1.2.so.0
#24 0x406544da in ?? () from /usr/lib/libgtk-1.2.so.0
#25 0x406f7ab2 in ?? () from /usr/lib/libgdk-1.2.so.0
#26 0x407212c6 in ?? () from /usr/lib/libglib-1.2.so.0
#27 0x40721801 in ?? () from /usr/lib/libglib-1.2.so.0
#28 0x40721979 in ?? () from /usr/lib/libglib-1.2.so.0
#29 0x40653f3a in ?? () from /usr/lib/libgtk-1.2.so.0
#30 0x405144c9 in ?? () from
/builds/seth/seamonkey/mozilla/dist/bin/libwidget_gtk.so
#31 0x4038dee1 in nsAppShellService::Run (this=0x809edf0) at
nsAppShellService.cpp:461
#32 0x409f77a6 in nsProfile::LoadDefaultProfileDir (this=0x811f698,
profileURLStr=@0xbffff8a4) at nsProfile.cpp:245
#33 0x409f73d7 in nsProfile::StartupWithArgs (this=0x811f698,
cmdLineArgs=0x8071678) at nsProfile.cpp:159
#34 0x804b22b in main1 (argc=2, argv=0xbffffa54) at nsAppRunner.cpp:548
#35 0x804b759 in main (argc=2, argv=0xbffffa54) at nsAppRunner.cpp:702
#36 0x4028bcb3 in ?? () from /lib/libc.so.6

Comment 1

[Mike McCabe]

Seth, shouldn't js_GetStringBytes assert str instead?  NULL might be a
reasonable value to return in a non-debug build, but I'm guessing that calling
js_GetStringBytes with a NULL argument is an indicator of some other problem
that we'd rather catch earlier rather than handle in this out of band way.

(Also, a Brendan-nit as this is Brendan code; consistent formatting puts the
if-body on a new line.)

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

JS_GetStringBytes should perhaps assert, but the patch is bogus.  You'd just
have to check the return value of JS_GetStringBytes instead of whatever is
giving you that NULL pointer.  If a JSAPI function is returning a NULL |JSString
*|, it's probably telling you that something went wrong.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So hang on a sec: what is js_ReportUncaughtException doing calling it with a
NULL pointer?  bad code! bad!

Comment 4

[(not reading, please use [e-mail] instead)]

accepting.  marking all milestone 11.

Comment 5

[(not reading, please use [e-mail] instead)]

what's wrong with bulletproofing js_GetStringBytes() to not crash if called with
null.

Something bad is probably happening, and after checking in the bulletproofing,
I can re-assign this bug to a JS person for inspection.

crashing is never a good thing.

Comment 6

[(not reading, please use [e-mail] instead)]

a different patch, but the same idea:
prevent js_GetStringBytes() from crashing, if it was passed a null.

Index: jsstr.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsstr.c,v
retrieving revision 3.12
diff -r3.12 jsstr.c
2381a2382
>
2394,2401c2395,2396
<           bytes = js_DeflateString(NULL, str->chars, str->length);
<           if (bytes) {
<               if (JS_HashTableRawAdd(cache, hep, hash, str, bytes)) {
<                   deflated_string_cache_bytes += str->length;
<               } else {
<                   free(bytes);
<                   bytes = NULL;
<               }
---
>           if (!str) {
>               bytes = NULL;
2402a2398,2408
>             else {
>                   bytes = js_DeflateString(NULL, str->chars, str->length);
>                   if (bytes) {
>                       if (JS_HashTableRawAdd(cache, hep, hash, str, bytes)) {
>                           deflated_string_cache_bytes += str->length;
>                       } else {
>                           free(bytes);
>                           bytes = NULL;
>                       }
>                   }
>          }

can I get one you js guys to review this, and then take over the bug and see why
js_GetStringBytes() was getting called with null?

Comment 7

[Brendan Eich [:brendan]]

Sorry, my code (it was once) does not null-check at every damn layer, just in
case someone screwed up a layer above, when all layers are within the JS engine.
That way lies performance death by a thousand cuts, and code bloat, and mental
slackness (no offense to sspitzer, who is new on scene).

All js_* routines are internal to the engine.  JS_* routines may well null-check
as they are relatively infrequent API calls -- but high-frequency ones may not,
again to hoist the stupidity tax out to callers.

In this case, the bug is in js_ReportUncaughtException, and belongs to mccabe.
Why didn't he take it?  Here it comes!

/be

Comment 8

[(not reading, please use [e-mail] instead)]

fair enough.  you JS guys run a tight ship.

The bug still exists (at least on Linux).  To reproduce, do this:

$ mv ~/.mozilla ~/.mozilla-old
$ ./mozilla-apprunner.sh

hit the new button to bring up the create profile wizard
hit next, then hit finished

crash.

Comment 9

[Mike McCabe]

Roger Lawrence is doing exception goo currently.  Roger, to you...

As far as not crashing on NULL...  Silently accepting NULL and returning is a
dual behavior, and a mistake.  Functions are best designed to do *one* thing.
Yes, maybe we shouldn't crash in this situation in a shipping product, but this
situation should be loudly flagged to developers.  Crashing is one way to raise
this flag.

Comment 10

[(not reading, please use [e-mail] instead)]

rogerl, if you need help getting set up to reproduce this, give me a call at
x4129.  you can come over to my cube and we can debug it.

Comment 11

[rogerl (gone)]

The problem is that damn null string - it "shouldn't" happen since it's origin
is a 'toString' call on an exception object.

sspitzer - thanks for the offer, if i can't reproduce it I'll stop by.

Comment 12

[rogerl (gone)]

*** Bug 15197 has been marked as a duplicate of this bug. ***

Comment 13

[(not reading, please use [e-mail] instead)]

*** Bug 15075 has been marked as a duplicate of this bug. ***

Comment 14

[rogerl (gone)]

An exception object is being created to reflect a JS error that has occurred but
the context that has been passed in is missing the exception prototype objects -
so the 'toString' method is not found and a null string created and ...

I don't understand why the context is missing the exception prototypes, they're
created as part of InitStandardClasses along with all the other native stuff and
yet are not available when the error occurs. The contxet being passed in looks
fine in all other regards. Is there any chance that a 'non-standard' context is
being used?

Comment 15

[(not reading, please use [e-mail] instead)]

fixed.  vidur's patch has been checked in with rogerl's blessing.

Comment 16

[Grace Bush]

builds for 19991007 Win and Linux
no more crashing!

Comment 17

[rogerl (gone)]

I want to re-open this since I think another bug is being hidden by the patch.
What's happenning is that the global object in the context being used to execute
the 'onclick' script has already been blown away by clearScope called from :

JS_ClearScope(JSContext * 0x01fe22e0, JSObject * 0x01c75960) line 2034 + 17
bytes
GlobalWindowImpl::SetNewDocument(GlobalWindowImpl * const 0x01fe24a4,
nsIDOMDocument * 0x00000000) line 286 + 32 bytes
DocumentViewerImpl::~DocumentViewerImpl() line 259
DocumentViewerImpl::`scalar deleting destructor'(unsigned int 0x00000001) + 15
bytes
DocumentViewerImpl::Release(DocumentViewerImpl * const 0x0200c1d0) line 217 + 99
bytes
nsWebShell::Destroy(nsWebShell * const 0x01fe1330) line 1161 + 27 bytes
nsWebShellWindow::Close(nsWebShellWindow * const 0x01fe18b0) line 454
GlobalWindowImpl::Close(GlobalWindowImpl * const 0x01fe24a8) line 1260
nsAppShellService::Quit(nsAppShellService * const 0x00d232b0) line 488
XPTC_InvokeByIndex(nsISupports * 0x00d232b0, unsigned int 0x00000005, unsigned
int 0x00000000, nsXPTCVariant * 0x0012d0cc) line 135
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x01fe22e0,
nsXPCWrappedNative * 0x01eaa930, const XPCNativeMemberDescriptor * 0x01ead1dc,
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 0x00000000, long *
0x01cf70e4, long * 0x0012d26c) line 767 + 44 bytes
WrappedNative_CallMethod(JSContext * 0x01fe22e0, JSObject * 0x01d26b48, unsigned
int 0x00000000, long * 0x01cf70e4, long * 0x0012d26c) line 186 + 34 bytes
js_Invoke(JSContext * 0x01fe22e0, unsigned int 0x00000000, unsigned int
0x00000000) line 672 + 26 bytes
js_Interpret(JSContext * 0x01fe22e0, long * 0x0012dae4) line 2248 + 15 bytes
js_Invoke(JSContext * 0x01fe22e0, unsigned int 0x00000000, unsigned int
0x00000000) line 688 + 13 bytes
js_Interpret(JSContext * 0x01fe22e0, long * 0x0012e318) line 2248 + 15 bytes
js_Invoke(JSContext * 0x01fe22e0, unsigned int 0x00000000, unsigned int
0x00000000) line 688 + 13 bytes
js_Interpret(JSContext * 0x01fe22e0, long * 0x0012eb4c) line 2248 + 15 bytes
js_Invoke(JSContext * 0x01fe22e0, unsigned int 0x00000001, unsigned int
0x00000000) line 688 + 13 bytes
js_Interpret(JSContext * 0x01fe22e0, long * 0x0012f380) line 2248 + 15 bytes
js_Invoke(JSContext * 0x01fe22e0, unsigned int 0x00000001, unsigned int
0x00000002) line 688 + 13 bytes
js_InternalCall(JSContext * 0x01fe22e0, JSObject * 0x01c74d98, long 0x01c74da0,
unsigned int 0x00000001, long * 0x0012f500, long * 0x0012f4b8) line 765 + 15
bytes
JS_CallFunction(JSContext * 0x01fe22e0, JSObject * 0x01c74d98, JSFunction *
0x0201f7f0, unsigned int 0x00000001, long * 0x0012f500, long * 0x0012f4b8) line
2650 + 32 bytes
nsJSContext::CallFunction(nsJSContext * const 0x01fe2450, void * 0x01c74d98,
void * 0x0201f7f0, unsigned int 0x00000001, void * 0x0012f500, int * 0x0012f4fc)
line 231 + 39 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x02108a80) line 103 + 48 bytes
nsEventListenerManager::HandleEvent(nsIPresContext & {...}, nsEvent *
0x0012f7f8, nsIDOMEvent * * 0x0012f7c0, unsigned int 0x00000007, nsEventStatus &
nsEventStatus_eIgnore) line 646 + 21 bytes
RDFElementImpl::HandleDOMEvent(RDFElementImpl * const 0x0201ff90, nsIPresContext
& {...}, nsEvent * 0x0012f7f8, nsIDOMEvent * * 0x0012f7c0, unsigned int
0x00000001, nsEventStatus & nsEventStatus_eIgnore) line 2894
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const
0x02023b20, nsIPresContext & {...}, nsMouseEvent * 0x0012fb6c, nsEventStatus &
nsEventStatus_eIgnore) line 1003 + 42 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x02023b20,
nsIPresContext & {...}, nsGUIEvent * 0x0012fb6c, nsIFrame * 0x02027ec0,
nsEventStatus & nsEventStatus_eIgnore, nsIView * 0x0200ea60) line 474 + 24 bytes
PresShell::HandleEvent(PresShell * const 0x0200e604, nsIView * 0x0200ea60,
nsGUIEvent * 0x0012fb6c, nsEventStatus & nsEventStatus_eIgnore) line 2102 + 43
bytes
nsView::HandleEvent(nsView * const 0x0200ea60, nsGUIEvent * 0x0012fb6c, unsigned
int 0x0000001c, nsEventStatus & nsEventStatus_eIgnore, int & 0x00000000) line
834
nsViewManager::DispatchEvent(nsViewManager * const 0x0200eb40, nsGUIEvent *
0x0012fb6c, nsEventStatus & nsEventStatus_eIgnore) line 1670
HandleEvent(nsGUIEvent * 0x0012fb6c) line 63
nsWindow::DispatchEvent(nsWindow * const 0x0200e924, nsGUIEvent * 0x0012fb6c,
nsEventStatus & nsEventStatus_eIgnore) line 342 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012fb6c) line 363
nsWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000) line
3306 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000)
line 3524
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long
0x01870078, long * 0x0012fd90) line 2533 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x033f0884, unsigned int 0x00000202, unsigned int
0x00000000, long 0x01870078) line 520 + 27 bytes
USER32! 77e71250()

Comment 18

[leger]

Clearing FIXED resolution due to reopen of this bug.

Comment 19

[Brendan Eich [:brendan]]

Adding vidur.  It looks like the event handler is quitting the app, which takes
down the JS environment of the running handler.  Maybe there's a way to avoid
clearing the window scope until after the handler finishes?

/be

Comment 20

[vidur (gone)]

Well...that was the question that I asked you in 15197, Brendan. Currently,
we're executing window.close() (and, hence, JS_ClearScope) synchronously.
Synchronous execution was probably not the case in 4.x because of the threading
model. It is the case in 4.x, however, that any script after the window.close()
doesn't execute (e.g. window.alert right after the close doesn't seem to
execute). It may be possible, with some work, to defer the actual closing of the
window (and, hence, the call to JS_ClearScope) till after script execution, but
I'm not sure that's correct either.

In the case of 15197, there was a call to setTimeout immediately after the
window.close(). It turns out that the call was inadvertant and really shouldn't
have been there. An exception was thrown since setTimeout no longer existed. The
patch that Roger checked in allowed the exception to be forwarded to the error
reporter successfully. It's unclear to me that this is worse than defering the
window closure and actually executing the setTimeout.

Comment 21

[Brendan Eich [:brendan]]

See http://lxr.mozilla.org/mozilla/source/lib/libmocha/lm_win.c#1418 for how
MozillaClassic solved this.  The next statement after window.close does not fail
to run in 4.x, for me:

<script>
foo=1
window.open("javascript:'<body onload=self.close();opener.foo=3><\/body>'")
</script>
<body onload='document.form.fooval.value = foo'>
<form name=form>
<input name=fooval size=4>
<input type=button onclick='document.form.fooval.value = foo'>
</form>
</body>

If you click the button after the opened window has closed itself, you'll see
that foo was set to 3.  If you're using alerts to trace execution, beware 4.x
does not put up an alert for a window being torn down, IIRC.

It seems to me we should fix this in Mozilla 5 using a setTimeout-like technique
to defer the window's destruction till after the handler.  It might be simpler
and safer to just set a flag that you check on end of script evaluation, though
-- what do you say?

/be

Comment 22

[vidur (gone)]

Ugh...the determination that the window.close() came from script (and from there
the determination that it's script in the same window) will be a treat. Other
than that, either setting or a timeout like mechanism could work.

Comment 23

[selmer (gone)]

Could javascript's window.close() bind to a function that helps resolve those
problems (or augment whatever it currently calls)?  Maybe pass along a window
handle or something?

Comment 24

[Jim Roskind]

Added dogfood and PDT+ annotations.

Comment 25

[vidur (gone)]

Fix checked in on 10/28/1999. If the calling context of a call to window.close()
is the same as the window's script context, the actual closure is defered till
the script completes. This is done by setting a termination function for the
script context.

Comment 26

[(not reading, please use [e-mail] instead)]

changing caps in summary.

Comment 27

[Grace Bush]

build 1999110508

Comment 28

[leger]

Moving all Profile Manager bugs to new Profile Manager Backend component.
Profile Manager component to be deleted.