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

VERIFIED FIXED in mozilla1.3alpha

Status

()

P1
critical
VERIFIED FIXED
16 years ago
8 years ago

People

(Reporter: dpasirst, Assigned: brendan)

Tracking

(4 keywords)

Trunk
mozilla1.3alpha
x86
All
crash, js1.5, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [QA note: verify interactively, in the browser and JS shell], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

16 years ago
Created attachment 105845 [details]
attachment refered to in inital bug report
(Reporter)

Updated

16 years ago
Attachment #105845 - Attachment mime type: application/octet-stream → application/zip
*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]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash

Comment 3

16 years ago
Created attachment 105860 [details]
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>
Attachment #105845 - Attachment is obsolete: true

Comment 4

16 years ago
Created attachment 105863 [details]
Stack trace Mozilla (old) build 20021010 on Linux

Updated

16 years ago
Keywords: regression, testcase
Summary: regression, crash on opening web page started earily in the 1.2 development cycle - see attachment → crash on opening web page started earily in the 1.2 development cycle [@ js_ValueToBoolean ] [@ js_ValueToString ]

Updated

16 years ago
OS: Windows NT → All

Updated

16 years ago
Summary: crash on opening web page started earily in the 1.2 development cycle [@ js_ValueToBoolean ] [@ js_ValueToString ] → crash on opening web page started early in the 1.2 development cycle [@ js_ValueToBoolean ] [@ js_ValueToString ]

Comment 5

16 years ago
-> JavaScript Engine
Assignee: jst → rogerl
Component: DOM HTML → JavaScript Engine
QA Contact: stummala → pschwartau

Comment 6

16 years ago
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
Keywords: js1.5

Comment 7

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

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

Comment 9

16 years ago
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 -
Whiteboard: [QA note: verify interactively, in the browser and JS shell]
(Assignee)

Comment 10

16 years ago
Created attachment 105941 [details] [diff] [review]
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
(Assignee)

Comment 11

16 years ago
My fault.

/be
Assignee: rogerl → brendan
Keywords: mozilla1.3
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Comment 12

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

/be
(Assignee)

Updated

16 years ago
Keywords: mozilla1.2

Comment 13

16 years ago
Comment on attachment 105941 [details] [diff] [review]
proposed fix

r=rogerl
Attachment #105941 - Flags: review+

Comment 14

16 years ago
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 on attachment 105941 [details] [diff] [review]
proposed fix

a=asa for checkin to the 1.2 branch (on behalf of drivers)
Attachment #105941 - Flags: approval+
(Assignee)

Comment 16

16 years ago
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
Status: NEW → ASSIGNED
(Assignee)

Comment 17

16 years ago
Fixed in the trunk and the 1.2 branch.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 18

16 years ago
> 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 -
(Assignee)

Comment 19

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

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

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

dpasirst@yahoo.com: if tomorrow's trunk build still crashes
on your testcase, please reopen this bug; thanks -
Status: RESOLVED → VERIFIED
(Reporter)

Comment 22

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

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

Updated

14 years ago
Flags: testcase+
Crash Signature: [@ js_ValueToBoolean ] [@ js_ValueToString ]
You need to log in before you can comment on or make changes to this bug.