179524 - crash on opening web page started early in the 1.2 development cycle [@ js_ValueToBoolean ] [@ js_ValueToString ]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dave Pasirstein]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021111
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021111

Pretty simple, mozilla crashed 100% of the time when opening similar pages.  It
started early in the 1.2 development cycle and has not yet been fixed.  See
attached zip file for the page that causes the crash.

Reproducible: Always

Steps to Reproduce:
1. unzip attached file
2. open test.html page in Mozilla
3.

Actual Results:  
Mozilla crashes

Expected Results:  
Mozilla should display the page

I work for a giant corporation and our standard intranet header/footer is based
on the same theme for the file I attached.  (I tried to sanitize the attachment.
I can assure you the real version is correct html; however, in either case,
Mozilla should not crash).  Previous versions of Netscape and Mozilla <= 1.1
work fine.

I am submitting this as Critial, but for me personally, it is a blocker.  I
simply cannot use Mozilla to access any intranet web site at my company.

I have submitted several talkback crashes mostly under other email addresses...
TB13796296Q or if you can search on my last name in the email field for
submitted talkbacks you will probably find many others.

Comment 1

[Dave Pasirstein]

Attachment: attachment refered to in inital bug report

Comment 2

[Matthias Versen [:Matti]]

*Incident ID:  *   
Incident ID 13796296
Stack Signature js_ValueToBoolean bd5a1ea9
Product ID MozillaTrunk
Build ID 2002111108
Trigger Time 2002-11-11 10:27:43
Platform Win32
Operating System Windows NT 4.0 build 1381
Module js3250.dll
URL visited
User Comments
Trigger Reason Access violation
Source File Name c:/builds/seamonkey/mozilla/js/src/jsbool.c
Trigger Line No. 223
Stack Trace

js_ValueToBoolean [c:/builds/seamonkey/mozilla/js/src/jsbool.c, line 223]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 1519]
js_Execute [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 1022]
JS_EvaluateUCScriptForPrincipals [c:/builds/seamonkey/mozilla/js/src/jsapi.c, 
line 3384]
nsJSContext::EvaluateString 
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 702]
nsScriptLoader::EvaluateScript 
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 586]
nsScriptLoader::ProcessRequest 
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 494]
nsScriptLoader::ProcessScriptElement 
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 438]
nsHTMLScriptElement::MaybeProcessScript 
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, 
line 699]
nsHTMLScriptElement::SetDocument 
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, 
line 499]
nsGenericHTMLContainerElement::AppendChildTo 
[c:/builds/seamonkey/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, 
line 4074]
HTMLContentSink::ProcessSCRIPTTag 
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp, 
line 5708]
HTMLContentSink::AddLeaf 
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp, 
line 3708]
CNavDTD::AddLeaf [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 
3806]
CNavDTD::HandleScriptToken 
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 2277]
CNavDTD::OpenContainer [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, 
line 3451]
CNavDTD::HandleDefaultStartToken 
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 1352]
CNavDTD::HandleStartToken 
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 1757]
CNavDTD::HandleToken [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, 
line 913]
CNavDTD::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, 
line 530]
nsParser::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, 
line 1890]
nsParser::ResumeParse [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, 
line 1754]
nsParser::ContinueParsing 
[c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 1371]
CSSLoaderImpl::Cleanup 
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp, line 1084]
CSSLoaderImpl::SheetComplete 
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp, line 1200]
CSSLoaderImpl::ParseSheet 
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp, line 1235]
CSSLoaderImpl::DidLoadStyle 
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp, line 1265]
SheetLoadData::OnStreamComplete 
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp, line 1022]
nsUnicharStreamLoader::OnStopRequest 
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsUnicharStreamLoader.cpp, line 187]
nsFileChannel::OnStopRequest 
[c:/builds/seamonkey/mozilla/netwerk/protocol/file/src/nsFileChannel.cpp, line 597]
nsOnStopRequestEvent::HandleEvent 
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 645]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, 
line 578]
_md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 
1336]

Comment 3

[Olav Vitters]

Attachment: Reduced testcase

Attaching reduced testcase, the following crashes 2002-11-11-08 Linux:

<script type="text/javascript">
var portalcontact="sss"
portalcontact.match("http://","i")
</script>

Comment 4

[Olivier Cahagne]

Attachment: Stack trace Mozilla (old) build 20021010 on Linux

Comment 5

[Olav Vitters]

-> JavaScript Engine

Comment 6

[Phil Schwartau]

Confirming crash in the current JS shell. It occurs whenever the 
user supplies more than one argument to String.prototype.match(): 

js> ''.match(/a/);
null
js> ''.match(/a/, '');
CRASH!!!!!!!!!!!!!!!!!!!!!!!!!!!
js> ''.match(/a/, '', '');
CRASH!!!!!!!!!!!!!!!!!!!!!!!!!!!


Of course, String.prototype.match() only takes ONE argument, and the
extra arguments are syntactically incorrect. But we shouldn't crash,
we should just ignore the extra arguments.

If the extra arguments are provided to Rhino, it does ignore them
and returns |null| for the match. SpiderMonkey used to do the same -


The stack traces for the JS shell crashes look like this:

js_ValueToString()
JS_ValueToString()
Process()
ProcessArgs()
main()
JS! mainCRTStartup
KERNEL32!


The crashpoint is indicated below:

js_ValueToString(JSContext *cx, jsval v)
{
    JSObject *obj;
    JSString *str;

    if (JSVAL_IS_OBJECT(v)) {
        obj = JSVAL_TO_OBJECT(v);
        if (!obj)
            return ATOM_TO_STRING(cx->runtime->atomState.nullAtom);
        if (!OBJ_DEFAULT_VALUE(cx, obj, JSTYPE_STRING, &v))
            return NULL;
    }
    if (JSVAL_IS_STRING(v)) {
        str = JSVAL_TO_STRING(v);
    } else if (JSVAL_IS_INT(v)) {
        str = js_NumberToString(cx, JSVAL_TO_INT(v));
    } else if (JSVAL_IS_DOUBLE(v)) {
        str = js_NumberToString(cx, *JSVAL_TO_DOUBLE(v));  <<<----------CRASH
    } else if (JSVAL_IS_BOOLEAN(v)) {
        str = js_BooleanToString(cx, JSVAL_TO_BOOLEAN(v));
    } else {
        str = ATOM_TO_STRING(cx->runtime->atomState.typeAtoms[JSTYPE_VOID]);
    }
    return str;
}


At the crashpoint, |v| = -623191334

Comment 7

[Phil Schwartau]

When I unzip the original testcase in Comment #1 and load
'test.html', I crash in this function, as indicated below:

JSBool
js_ValueToBoolean(JSContext *cx, jsval v, JSBool *bp)
{
    JSBool b;
    jsdouble d;

       etc.

    if (JSVAL_IS_DOUBLE(v)) {
	d = *JSVAL_TO_DOUBLE(v);   <<<------------- CRASH
	b = (!JSDOUBLE_IS_NaN(d) && d != 0) ? JS_TRUE : JS_FALSE;
    } ELSE


       etc.
}


As in Comment #6 above, |v| = -623191334

Comment 8

[Andrew Schultz]

regression between linux trunk builds 2002092221 and 2002092408, which means bug
167658

Comment 9

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-179524.js

However, it is not sufficient to expose the current bug. So as with
bug 167658, any fix will have to be verified interactively in the
browser or the JS shell.

The code in the testcase does crash the current JS shell if I key it 
interactively. But if the testcase is run via load() or the -f option
to the JS shell, it doesn't crash -

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed fix

Phil, can you also test the case where match's first argument is not a regexp
object?  In that case, we follow ECMA-262 Ed. 3 by converting the 1st argument
to  a string taken as regexp literal source, but then we extend ECMA by taking
any optional 2nd argument to be a regexp flag string (e.g., 'g').  We need to
test both the 1st-arg-is-regexp and 1st-arg-is-string cases, and for the second
case, the optional flag argument too.

/be

Comment 11

[Brendan Eich [:brendan]]

My fault.

/be

Comment 12

[Brendan Eich [:brendan]]

Reviews wanted fast -- I'd like to get this fix into 1.2 if possible.

/be

Comment 13

[rogerl (gone)]

Comment on attachment 105941 [details] [diff] [review]
proposed fix

r=rogerl

Comment 14

[Phil Schwartau]

Brendan: thanks, I have added sections to the testcase covering the 
1st-arg-is-string case. I put in cases where the optional 2nd argument
is not provided, is a valid regexp flag, or is an invalid regexp flag.
I also threw in some extraneous 3rd arguments, etc.

I did not realize this syntax existed! Thank you -

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 105941 [details] [diff] [review]
proposed fix

a=asa for checkin to the 1.2 branch (on behalf of drivers)

Comment 16

[Brendan Eich [:brendan]]

We support the optional flags trailing parameter for search and replace as well
as for match.  Phil, did you make tests for those methods too?

/be

Comment 17

[Brendan Eich [:brendan]]

Fixed in the trunk and the 1.2 branch.

/be

Comment 18

[Phil Schwartau]

> We support the optional flags trailing parameter for search and replace
> as well as for match.  Phil, did you make tests for those methods too?

I have added coverage for this to the above testcase. Note that when
testing str.replace(), we have to be careful if the first argument is
not a regexp object. ECMA-262 Edition 3 says it is NOT converted to one,
UNLIKE the case for str.match(), str.search(). See bug 83293 comment #21.

This means we have to be careful how we test meta-characters in the
first argument to str.replace(), if that argument is a string -

Comment 19

[Brendan Eich [:brendan]]

Phil: right, we force any non-regexp 1st param to replace to be a flat string to
match, but we use the regexp machinery to do that still, so the flags (optional,
an extension to ECMA) provide global-replace and case-insensitive matching, if
the user wants.  We should test those extensions, of course!

/be

Comment 20

[Phil Schwartau]

> We should test those extensions, of course!

Right - I've included that in the above testcase. I just had to be careful
when testing str.replace() not to supply meta-chars to the 1st parameter:

i.e. not this:   actual = str.replace('\\s', 'Z', 'm');
                 expect = str.replace(/\s/m, 'Z');
but this is OK:
                 actual = str.replace(' ', 'Z', 'm');
                 expect = str.replace(/ /m, 'Z');


It caught me by surprise - because with str.search() and str.match(),
there is no such issue.

Comment 21

[Phil Schwartau]

Marking Verified FIXED.

I checked all 36 sections of the above testcase interactively,
in both the debug and optimized JS shell on WinNT.

I also verified, via bonsai, identical modifications to jsstr.c
in the trunk and -rMOZILLA_1_2_BRANCH in CVS.

[e-mail]: if tomorrow's trunk build still crashes
on your testcase, please reopen this bug; thanks -

Comment 22

[Dave Pasirstein]

Great Job to all!!!  I was amazed at the responsiveness in fixing this bug.

I verified it as fixed today in the nightlies for both the trunk (Build ID:
2002111304) and 1.2 branch (Build ID: 2002111214).  All looks great, the crash
is gone :-)

Thank you very much!

Comment 23

[Olivier Cahagne]

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